Skip to content

Conversation

@pedrobsaila
Copy link
Contributor

Fixes #88873

@ghost ghost added area-Extensions-Configuration community-contribution Indicates that the PR has been added by a community member labels Aug 1, 2023
@ghost
Copy link

ghost commented Aug 1, 2023

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #88873

Author: pedrobsaila
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@tarekgh tarekgh requested a review from layomia August 1, 2023 23:30
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Thanks @pedrobsaila. LGTM

@tarekgh tarekgh merged commit 086ac0a into dotnet:main Aug 2, 2023
Copy link
Contributor

@layomia layomia left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @pedrobsaila! Pls address these comments in a follow up PR if you get a chance.

EmitBlankLineIfRequired();
EmitPrimitiveParseMethod(type);

if (type.StringParsableTypeKind == StringParsableTypeKind.Enum)
Copy link
Contributor

@layomia layomia Aug 2, 2023

Choose a reason for hiding this comment

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

I think the parser should not include enum types in the PrimitivesForHelperGen list. The emitter should have minimal/no post-processing of the input data. The SourceGenerationSpec could have a boolean property indicating whether an enum was part of the input graph.

parsedValueExpr = $"({typeDisplayString}){Identifier.Enum}.{Identifier.Parse}(typeof({typeDisplayString}), {Identifier.value}, ignoreCase: true)";
}
break;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should never fall into this case based on other diffs above.

@layomia
Copy link
Contributor

layomia commented Aug 3, 2023

Created #89936 to track the follow-up fixes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Extensions-Configuration community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve enum parsing in config binder generator

5 participants