Skip to content

Conversation

@shuheiktgw
Copy link
Collaborator

@shuheiktgw shuheiktgw commented May 23, 2025

Closes #741

Previously, decoding string named type fields returned the converted string values directly, which could lead to a panic if the types didn’t match. This update adds a type check and performs the conversion if the types do not match.

Before submitting your PR, please confirm the following.

  • Describe the purpose for which you created this PR.
  • Create test code that corresponds to the modification
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.85%. Comparing base (04f9bb5) to head (74d87cd).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #743      +/-   ##
==========================================
+ Coverage   77.83%   77.85%   +0.02%     
==========================================
  Files          22       22              
  Lines        8056     8064       +8     
==========================================
+ Hits         6270     6278       +8     
  Misses       1370     1370              
  Partials      416      416              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
@shuheiktgw shuheiktgw requested a review from Copilot May 23, 2025 07:06
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for decoding YAML values into string-alias types without panicking, along with corresponding tests.

  • Introduce TestString alias and new test cases for mapping both unquoted and numeric YAML values into TestString.
  • Refactor convertValue to collect primitive values into a strVal before converting to the target type, then special-case named string types.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
decode_test.go Adds TestString alias and table-driven tests for mapping YAML values into TestString.
decode.go Refactors convertValue to accumulate a string buffer, then convert to alias string types.
Comments suppressed due to low confidence (2)

decode.go:637

  • This condition will return early for non-string kinds (e.g. int) when targeting a string alias, causing a panic instead of going through the intended numeric-to-string conversion path. Consider changing to v.Type().Kind() == reflect.String or removing this early return so that numeric and boolean kinds hit the switch and get formatted into strings first.
if typ.Kind() == reflect.String && v.Type().Kind() != reflect.String {

decode_test.go:40

  • Tests cover string and numeric inputs for TestString, but boolean and float values for the alias are not verified. Adding cases like source: "v: true\n", value: map[string]TestString{"v": "true"} and a float test will ensure full coverage of all primitive-to-string conversions.
{
Copy link
Owner

@goccy goccy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your great PR ! LGTM

@goccy goccy merged commit 6a6c998 into master May 26, 2025
25 checks passed
@goccy goccy deleted the panic_named_types branch May 26, 2025 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants