Fix regression in stringer enum parsing

[vearutop]

Fixes #128.

[github-actions]

Lines Of Code

Language	Files	Lines	Code	Comments	Blanks	Complexity	Bytes
Go	7	2475 (+16)	1697 (+12)	274	504 (+4)	657 (+3)	59.6K (+235B)
Go (test)	13	4269 (+35)	3181 (+28)	311	777 (+7)	54 (+3)	108.1K (+655B)

[github-actions]

Go API Changes

# summary
Inferred base version: v0.3.77
Suggested version: v0.3.78

[github-actions]

Unit Test Coverage

total: (statements) 79.9%
changed lines: (statements) 73.9%, coverage is less than 90.0%, consider testing the changes more thoroughly

Coverage of changed lines

File	Function	Coverage
Total		73.9%
reflect.go		73.9%
reflect.go:546	isTextMarshaler	100.0%
reflect.go:557	checkTextMarshaler	100.0%
reflect.go:1475	inferType	73.9%

Coverage diff with base branch

File	Function	Base Coverage	Current Coverage
Total		79.8%	79.9% (+0.1%)
reflect.go	inferType	60.0%	68.4% (+8.4%)
reflect.go	isTextMarshaler	no function	100.0%

[github-actions]

Benchmark Result

Benchmark diff with base branch

name                        old time/op    new time/op    delta
Schema_UnmarshalJSON_raw-4    58.4µs ± 0%    59.5µs ± 2%    ~     (p=0.063 n=4+5)
Schema_UnmarshalJSON-4         462µs ± 0%     477µs ± 1%  +3.05%  (p=0.016 n=4+5)
Schema_MarshalJSON_raw-4      39.0µs ± 1%    39.5µs ± 1%  +1.31%  (p=0.032 n=5+5)
Schema_MarshalJSON-4           185µs ± 1%     188µs ± 0%  +1.68%  (p=0.008 n=5+5)

name                        old alloc/op   new alloc/op   delta
Schema_UnmarshalJSON_raw-4    31.3kB ± 0%    31.3kB ± 0%    ~     (all equal)
Schema_UnmarshalJSON-4         180kB ± 0%     180kB ± 0%    ~     (all equal)
Schema_MarshalJSON_raw-4      14.6kB ± 0%    14.6kB ± 0%    ~     (p=0.238 n=5+4)
Schema_MarshalJSON-4          53.9kB ± 0%    53.9kB ± 0%    ~     (p=0.397 n=5+5)

name                        old allocs/op  new allocs/op  delta
Schema_UnmarshalJSON_raw-4       460 ± 0%       460 ± 0%    ~     (all equal)
Schema_UnmarshalJSON-4         1.85k ± 0%     1.85k ± 0%    ~     (all equal)
Schema_MarshalJSON_raw-4         370 ± 0%       370 ± 0%    ~     (all equal)
Schema_MarshalJSON-4             468 ± 0%       468 ± 0%    ~     (all equal)

Benchmark result

name                        time/op
Schema_UnmarshalJSON_raw-4  59.5µs ± 2%
Schema_UnmarshalJSON-4       477µs ± 1%
Schema_MarshalJSON_raw-4    39.5µs ± 1%
Schema_MarshalJSON-4         188µs ± 0%

name                        alloc/op
Schema_UnmarshalJSON_raw-4  31.3kB ± 0%
Schema_UnmarshalJSON-4       180kB ± 0%
Schema_MarshalJSON_raw-4    14.6kB ± 0%
Schema_MarshalJSON-4        53.9kB ± 0%

name                        allocs/op
Schema_UnmarshalJSON_raw-4     460 ± 0%
Schema_UnmarshalJSON-4       1.85k ± 0%
Schema_MarshalJSON_raw-4       370 ± 0%
Schema_MarshalJSON-4           468 ± 0%

[llamapreview]

Auto Pull Request Review from LlamaPReview

1. Overview

1.1 Core Changes

• Primary purpose and scope: Fix regression in stringer enum parsing
• Key components modified: reflect.go and reflect_test.go
• Cross-component impacts: Corrects schema generation for text-marshaler enums
• Business value alignment: Aligns with the need to maintain accurate JSON schema mappings for Go types, ensuring compatibility and correctness for users relying on stringer-generated enums

1.2 Technical Architecture

• System design modifications: N/A
• Component interaction changes: N/A
• Integration points impact: N/A
• Dependency changes and implications: N/A

2. Critical Findings

2.1 Must Fix (P0🔴)

[No critical issues were found in the PR. The changes resolve the reported regression and pass existing CI tests.]

2.2 Should Fix (P1🟡)

Issue: Silent Skipping of Invalid Enum Values

• Analysis Confidence: High
• Impact: Non-text-marshaler enums with invalid values (e.g., "abc" for int) are omitted without warnings, potentially hiding user errors.
• Suggested Solution: Add validation warnings (e.g., via logging) when enum values fail parsing.

2.3 Consider (P2🟢)

Area: Boolean Enum Handling

• Analysis Confidence: High
• Improvement Opportunity: The current code mishandles bool enums (e.g., enum:"true,false" generates string values instead of booleans). Add a case for reflect.Bool in inferType to parse values as booleans.

Area: Edge Case Testing

• Analysis Confidence: High
• Improvement Opportunity: Add tests for invalid enum values (e.g., int field with enum:"1,abc,3") to confirm invalid entries are skipped.

Area: Documentation Clarity

• Analysis Confidence: High
• Improvement Opportunity: Explicitly state that enum tag values must match MarshalText outputs for text-marshaler types.

2.4 Summary of Action Items

• P1: Implement validation warnings for invalid enum values.
• P2: Address boolean enum handling, add edge case testing, and improve documentation clarity.

3. Technical Analysis

3.1 Code Logic Analysis

📁 reflect.go - inferType

• Submitted PR Code:

    func (enum *enum) inferType(enumTag string, fv reflect.Value) []interface{} {
        es := strings.Split(enumTag, ",")
    -  e := make([]interface{}, len(es))
    +  e := make([]interface{}, 0, len(es))
    +
    +  if isTextMarshaler(fv.Type()) {
    +      for _, s := range es {
    +          e = append(e, s)
    +      }
    +      return e
    +  }

• Analysis:
  • The current logic skips numeric parsing for text-marshalers, preventing null values.
  • Edge cases and error handling are considered, but silent skipping of invalid enum values could hide user errors.
  • Cross-component impact: N/A
  • Business logic considerations: N/A
• LlamaPReview Suggested Improvements:

    func (enum *enum) inferType(enumTag string, fv reflect.Value) []interface{} {
        es := strings.Split(enumTag, ",")
        e := make([]interface{}, 0, len(es))

        if isTextMarshaler(fv.Type()) {
            for _, s := range es {
                e = append(e, s)
            }
            return e
        }

        // Add validation warnings for invalid enum values
        for _, s := range es {
            if v, err := strconv.ParseInt(s, 10, 64); err == nil {
                e = append(e, v)
            } else {
                log.Printf("Warning: Invalid enum value '%s' for type %s", s, fv.Type())
            }
        }

        return e
    }

• Improvement rationale:
  • Technical benefits: Adds validation warnings for invalid enum values, improving debuggability.
  • Business value: Prevents subtle schema mismatches.
  • Risk assessment: Low risk, as it only adds logging and does not change the core functionality.

📁 reflect_test.go - TestReflector_Reflect_enum

• Submitted PR Code:

    Stringer MyEnum `json:"stringer" enum:"On,Off"`

• Analysis:
  • The test addition validates that the fix produces ["On","Off"] in the schema, ensuring regression resistance.
  • Edge cases and error handling are considered.
  • Cross-component impact: N/A
  • Business logic considerations: N/A
• LlamaPReview Suggested Improvements: N/A
• Improvement rationale: N/A

3.2 Key Quality Aspects

• System scalability considerations: N/A
• Performance bottlenecks and optimizations: N/A
• Testing strategy and coverage: The PR includes a test case for MyEnum to validate correct string-based enum generation. Additional edge case testing is suggested.
• Documentation needs: Explicitly state that enum tag values must match MarshalText outputs for text-marshaler types.

4. Overall Evaluation

• Technical assessment: The PR addresses the regression effectively with a targeted fix and maintains code clarity.
• Business impact: Restores correct enum parsing, ensuring accurate JSON schema mappings for users.
• Risk evaluation: Low risk, as the changes are well-scoped and tested.
• Notable positive aspects and good practices: Precise fix, maintainable code structure, effective test coverage.
• Implementation quality: High quality, with a clear and concise implementation.
• Final recommendation: Approve

---

💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.