Skip to content

Address feedback from config binding gen PR to improve enum parsing - #89952

Merged
tarekgh merged 22 commits into
dotnet:mainfrom
pedrobsaila:89936
Oct 11, 2023
Merged

Address feedback from config binding gen PR to improve enum parsing#89952
tarekgh merged 22 commits into
dotnet:mainfrom
pedrobsaila:89936

Conversation

@pedrobsaila

@pedrobsailapedrobsaila commented Aug 3, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#89936
Fixes#89879

@layomia I addressed the follow-up fixes.

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

ghost commented Aug 3, 2023

Copy link
Copy Markdown

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

Issue Details

Fixes #89936

@layomia I addressed 2 of the follow-up fixes. I'll wait for the final decision (polyfill/checking whether Enum.Parse is available) before proceeding to fix it

Author:pedrobsaila
Assignees:-
Labels:

area-Extensions-Configuration

Milestone:-

@build-analysisbuild-analysisBot mentioned this pull request Aug 3, 2023
Closed
layomia
layomia reviewed Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs Outdated

@layomialayomiaAug 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd re-write this as

Suggested change
if(stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.AssignFromSectionValue&&stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.Enum)
if(specialTypeKindisStringParsableTypeKind.Enum)
{
_sourceGenSpec.EmitEnumParseMethod=true;
}
elseif(specialTypeKindis not StringParsableTypeKind.AssignFromSectionValue)
{
_sourceGenSpec.PrimitivesForHelperGen.Add(stringParsableSpec);
}

layomia
layomia reviewed Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs Outdated
Comment on lines 68 to 67

@layomialayomiaAug 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't need this given the suggestion below.

@layomia

layomia commented Aug 9, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

@pedrobsaila, let's go with checking for the right method in the parser - #89823 (comment). We could also address #89879 in this PR.

@build-analysisbuild-analysisBot mentioned this pull request Aug 12, 2023
Closed
3 tasks
@layomialayomia self-assigned this Aug 25, 2023
@layomia

layomia commented Aug 25, 2023

Copy link
Copy Markdown
Contributor

@pedrobsaila apologies for my delayed response. Could you pls resolve merge conflicts here?

@build-analysisbuild-analysisBot mentioned this pull request Aug 27, 2023
Closed
layomia
layomia reviewed Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs Outdated
ISet_Unbound = ISet?.ConstructUnboundGenericType();

// needed to be able to know if a member exist inside the compilation unit
Enum = compilation.GetBestTypeByMetadataName(typeof(Enum));

@layomialayomiaAug 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use the overload taking a SpecialType (i.e. SpecialType.System_Enum). All the special types are guaranteed to be in the input compilation.

layomia
layomia reviewed Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs Outdated
public INamedTypeSymbol? ISet_Unbound { get; }
public INamedTypeSymbol? ISet { get; }
public INamedTypeSymbol? List { get; }
public INamedTypeSymbol? Enum { get; }

@layomialayomiaAug 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
publicINamedTypeSymbol?Enum{get;}
publicINamedTypeSymbolEnum{get;}

Per https://github.com/dotnet/runtime/pull/89952/files#r1307693295 the fetched value would not be null.

layomia
layomia reviewed Aug 28, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs Outdated
}
}

_sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;

@layomialayomiaAug 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
_sourceGenSpec.EmitThrowIfNullMethod=!_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod=_typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmptyisfalse;

The symbol could be null; avoid a null-ref exception.

layomia
layomia reviewed Aug 28, 2023
View reviewed changes
Comment thread...der/tests/SourceGenerationTests/Baselines/ServiceCollection/Configure_T.net462.generated.txt
@@ -0,0 +1,243 @@
// <auto-generated/>

@layomialayomiaAug 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the net462 variants. Could we refactor to the following structure?

> baselines
> netcoreapp
> foo.generated.txt
> net462
> foo.generated.txt

I'd actually suggest making a preliminary PR doing this. We'd need to remove the condition on [this line] so that the net462 tests would be included in automated runs. Doing this will reveal a bit of additional, tangential work needed to complete the effort.

@layomia

layomia commented Aug 28, 2023

Copy link
Copy Markdown
Contributor

Oof looks like merging #91180 reset your rebase effort. If you take another stab, I'll be sure to check if this PR is merge-ready & queue it up first if so.

layomia
layomia reviewed Sep 11, 2023
View reviewed changes
Comment thread...ries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs Outdated
ExtensionClassType extType = ExtensionClassType.None)
Action<ImmutableArray<Diagnostic>>? assessDiagnostics = null)
{
string path = extType is ExtensionClassType.None

@layomialayomiaSep 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use a a preprocessor directive to determine the path

stringpath= #if NETCOREAPP
...
#else ...;

layomia
layomia reviewed Sep 11, 2023
View reviewed changes
Comment thread...soft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.Baselines.cs Outdated
{
string source = GetCollectionsSource();

await VerifyAgainstBaselineUsingFile("Collections.net462.generated.txt", source, assessDiagnostics: (d) =>

@layomialayomiaSep 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Per f5a2abc#r1321885499, please revert the changes to these invocations.

layomia
layomia reviewed Sep 11, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj Outdated
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
<Content Include="Baselines\*.generated.txt;

@layomialayomiaSep 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please revert back to partitioning the baselines based on the binder ext. type. This is helpful when working issues specific to a particular binder.

layomia
layomia reviewed Sep 11, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs Outdated
}

_sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException is not null && _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty is false;

@layomialayomiaSep 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmpty is false;

Also -- to be doubly sure, check that the method has the expected parameters.

@pedrobsaila

pedrobsaila commented Sep 18, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@layomia can you check it please before the PR become stale again

@eiriktsarpalis

eiriktsarpalis commented Oct 4, 2023

Copy link
Copy Markdown
Member

@pedrobsaila apologies for the delayed response, would it be possible to rebase your changes? Thanks!

@tarekgh

tarekgh commented Oct 5, 2023•
edited
Loading

Copy link
Copy Markdown
Member

@pedrobsaila do you have a chance to resolve the merging conflicts?

@pedrobsaila

pedrobsaila commented Oct 6, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I'm on it. The code in main branch changed deeply, resolving conflict is not easy and will take some time

@pedrobsaila

pedrobsaila commented Oct 7, 2023

Copy link
Copy Markdown
ContributorAuthor

@eiriktsarpalis@tarekgh ready for a new round of reviews

tarekgh
tarekgh reviewed Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj Outdated
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
<Content Include="Baselines\*.generated.txt;Baselines\ConfigurationBinder\*.generated.txt;Baselines\OptionsBuilder\*.generated.txt;Baselines\ServiceCollection\*.generated.txt">
<ItemGroup>
<Content Include="Baselines\net462\*.generated.txt;

@tarekghtarekghOct 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Baselines

do we need to copy specific files? can't we include the whole Baseline folder?

@pedrobsailapedrobsailaOct 10, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Include just Baselines does not seem to work, the max I could do is this lightweight version :

<Content Include="Baselines\*\*\*.generated.txt;
Baselines\*\*.generated.txt;">

@tarekghtarekghOct 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can use the syntax

runtime/src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Microsoft.Extensions.Options.SourceGeneration.Tests.csproj

Line 42 in c9b5dd4

<ContentInclude="Baselines\**\*">

@tarekgh

tarekgh commented Oct 10, 2023

Copy link
Copy Markdown
Member

@pedrobsaila thanks for getting this ready. I added a few questions if you can help with. otherwise LGTM.

tarekgh
tarekgh reviewed Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj Outdated
Baselines\netcoreapp\ConfigurationBinder\*.generated.txt;
Baselines\netcoreapp\OptionsBuilder\*.generated.txt;
Baselines\netcoreapp\ServiceCollection\*.generated.txt;">
<Content Include="Baselines\*\*\*.generated.txt;

@tarekghtarekghOct 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this doesn't look right.

@pedrobsailapedrobsailaOct 10, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

seems right to me. There are 2 patterns of files :

  • Baselines\netcoreapp\OptionsBuilder\*.generated.txt => would match Baselines\*\*\*.generated.txt
  • Baselines\netcoreapp\Primitives.generated.txt => would match Baselines\*.generated.txt

@tarekghtarekghOct 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we can copy the whole Baseline as I pointed out in #89952 (comment) that will be better I guess.

tarekgh
tarekgh approved these changes Oct 11, 2023
View reviewed changes

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

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 f1b4930 into dotnet:mainOct 11, 2023
@pedrobsaila
pedrobsaila deleted the 89936 branch October 11, 2023 20:45
@ghostghost locked as resolved and limited conversation to collaborators Nov 11, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tarekghtarekghtarekgh approved these changes
@eiriktsarpaliseiriktsarpalisAwaiting requested review from eiriktsarpalis
@ericstjericstjAwaiting requested review from ericstj
+1 more reviewer
@layomialayomialayomia left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Extensions-Configurationcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

4 participants

@pedrobsaila@layomia@eiriktsarpalis@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Address feedback from config binding gen PR to improve enum parsing by pedrobsaila · Pull Request #89952 · dotnet/runtime · GitHub
Skip to content
Address feedback from config binding gen PR to improve enum parsing - #89952
Merged
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Oct 11, 2023
Merged
Address feedback from config binding gen PR to improve enum parsing#89952
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Conversation
@pedrobsaila
@pedrobsailapedrobsaila
commented
Aug 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#89936
Fixes#89879
@layomia I addressed the follow-up fixes.
@ghostghost
added

area-Extensions-Configuration

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 3, 2023
@ghost
ghost
commented
Aug 3, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-configuration

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89936
@layomia I addressed 2 of the follow-up fixes. I'll wait for the final decision (polyfill/checking whether Enum.Parse is available) before proceeding to fix it
Author:pedrobsaila
Assignees:-
Labels:
area-Extensions-Configuration
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed

layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'd re-write this as

Suggested change

if(stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.AssignFromSectionValue&&stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.Enum)
if(specialTypeKindisStringParsableTypeKind.Enum)
{
_sourceGenSpec.EmitEnumParseMethod=true;
}
elseif(specialTypeKindis not StringParsableTypeKind.AssignFromSectionValue)
{
_sourceGenSpec.PrimitivesForHelperGen.Add(stringParsableSpec);
}
layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated


Comment on lines

68
 to 
67

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't need this given the suggestion below.
@layomia
layomia
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@pedrobsaila, let's go with checking for the right method in the parser - #89823 (comment). We could also address #89879 in this PR.
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 12, 2023

Closed

3 tasks
@layomialayomia
self-assigned this
Aug 25, 2023
@layomia
layomia
commented
Aug 25, 2023

Copy link


Copy Markdown

Contributor
@pedrobsaila apologies for my delayed response. Could you pls resolve merge conflicts here?
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 27, 2023

Closed

layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 ISet_Unbound = ISet?.ConstructUnboundGenericType();

 // needed to be able to know if a member exist inside the compilation unit
 Enum = compilation.GetBestTypeByMetadataName(typeof(Enum));
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Use the overload taking a SpecialType (i.e. SpecialType.System_Enum). All the special types are guaranteed to be in the input compilation.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 public INamedTypeSymbol? ISet_Unbound { get; }
 public INamedTypeSymbol? ISet { get; }
 public INamedTypeSymbol? List { get; }
 public INamedTypeSymbol? Enum { get; }
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

publicINamedTypeSymbol?Enum{get;}
publicINamedTypeSymbolEnum{get;}
Per https://github.com/dotnet/runtime/pull/89952/files#r1307693295 the fetched value would not be null.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }
 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

_sourceGenSpec.EmitThrowIfNullMethod=!_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod=_typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmptyisfalse;
The symbol could be null; avoid a null-ref exception.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...der/tests/SourceGenerationTests/Baselines/ServiceCollection/Configure_T.net462.generated.txt
@@ -0,0 +1,243 @@
// <auto-generated/>
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for adding the net462 variants. Could we refactor to the following structure?
> baselines
> netcoreapp
> foo.generated.txt
> net462
> foo.generated.txt

I'd actually suggest making a preliminary PR doing this. We'd need to remove the condition on [this line] so that the net462 tests would be included in automated runs. Doing this will reveal a bit of additional, tangential work needed to complete the effort.
@layomia
layomia
commented
Aug 28, 2023

Copy link


Copy Markdown

Contributor
Oof looks like merging #91180 reset your rebase effort. If you take another stab, I'll be sure to check if this PR is merge-ready & queue it up first if so.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs
Outdated

 ExtensionClassType extType = ExtensionClassType.None)
 Action<ImmutableArray<Diagnostic>>? assessDiagnostics = null)
 {
 string path = extType is ExtensionClassType.None
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please use a a preprocessor directive to determine the path
stringpath= #if NETCOREAPP
...
#else ...;
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...soft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.Baselines.cs
Outdated

 {
 string source = GetCollectionsSource();

 await VerifyAgainstBaselineUsingFile("Collections.net462.generated.txt", source, assessDiagnostics: (d) =>
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Per f5a2abc#r1321885499, please revert the changes to these invocations.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please revert back to partitioning the baselines based on the binder ext. type. This is helpful when working issues specific to a particular binder.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
 _sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException is not null && _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty is false;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmpty is false;
Also -- to be doubly sure, check that the method has the expected parameters.
@pedrobsaila
pedrobsaila
commented
Sep 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@layomia can you check it please before the PR become stale again
@eiriktsarpalis
eiriktsarpalis
commented
Oct 4, 2023

Copy link


Copy Markdown

Member
@pedrobsaila apologies for the delayed response, would it be possible to rebase your changes? Thanks!
@tarekgh
tarekgh
commented
Oct 5, 2023•

edited

Loading

Copy link


Copy Markdown

Member
@pedrobsaila do you have a chance to resolve the merging conflicts?
@pedrobsaila
pedrobsaila
commented
Oct 6, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I'm on it. The code in main branch changed deeply, resolving conflict is not easy and will take some time
@pedrobsaila
pedrobsaila
commented
Oct 7, 2023

Copy link


Copy Markdown

ContributorAuthor
@eiriktsarpalis@tarekgh ready for a new round of reviews
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;Baselines\ConfigurationBinder\*.generated.txt;Baselines\OptionsBuilder\*.generated.txt;Baselines\ServiceCollection\*.generated.txt">
 <ItemGroup>
 <Content Include="Baselines\net462\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

"Baselines 
do we need to copy specific files? can't we include the whole Baseline folder?
@pedrobsailapedrobsailaOct 10, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Include just Baselines does not seem to work, the max I could do is this lightweight version :
<Content Include="Baselines\*\*\*.generated.txt;
Baselines\*\*.generated.txt;">

@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You can use the syntax 
runtime/src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Microsoft.Extensions.Options.SourceGeneration.Tests.csproj

Line 42
in
c9b5dd4
 <ContentInclude="Baselines\**\*"> 
@tarekgh
tarekgh
commented
Oct 10, 2023

Copy link


Copy Markdown

Member
@pedrobsaila thanks for getting this ready. I added a few questions if you can help with. otherwise LGTM.
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 Baselines\netcoreapp\ConfigurationBinder\*.generated.txt;
 Baselines\netcoreapp\OptionsBuilder\*.generated.txt;
 Baselines\netcoreapp\ServiceCollection\*.generated.txt;">
 <Content Include="Baselines\*\*\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

this doesn't look right.
@pedrobsailapedrobsailaOct 10, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

seems right to me. There are 2 patterns of files :
  • Baselines\netcoreapp\OptionsBuilder\*.generated.txt => would match Baselines\*\*\*.generated.txt
  • Baselines\netcoreapp\Primitives.generated.txt => would match Baselines\*.generated.txt
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we can copy the whole Baseline as I pointed out in #89952 (comment) that will be better I guess.
tarekgh
tarekgh
approved these changes
Oct 11, 2023
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
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 f1b4930
into
dotnet:mainOct 11, 2023
@pedrobsaila
pedrobsaila
deleted the

89936

branch
October 11, 2023 20:45
@ghostghost
locked as resolved and limited conversation to collaborators
Nov 11, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghtarekgh approved these changes
@eiriktsarpaliseiriktsarpalisAwaiting requested review from eiriktsarpalis
@ericstjericstjAwaiting requested review from ericstj

+1 more reviewer

@layomialayomialayomia left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Extensions-Configurationcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


4 participants

@pedrobsaila@layomia@eiriktsarpalis@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Address feedback from config binding gen PR to improve enum parsing by pedrobsaila · Pull Request #89952 · dotnet/runtime · GitHub
Skip to content
Address feedback from config binding gen PR to improve enum parsing - #89952
Merged
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Oct 11, 2023
Merged
Address feedback from config binding gen PR to improve enum parsing#89952
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Conversation
@pedrobsaila
@pedrobsailapedrobsaila
commented
Aug 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#89936
Fixes#89879
@layomia I addressed the follow-up fixes.
@ghostghost
added

area-Extensions-Configuration

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 3, 2023
@ghost
ghost
commented
Aug 3, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-configuration

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89936
@layomia I addressed 2 of the follow-up fixes. I'll wait for the final decision (polyfill/checking whether Enum.Parse is available) before proceeding to fix it
Author:pedrobsaila
Assignees:-
Labels:
area-Extensions-Configuration
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed

layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'd re-write this as

Suggested change

if(stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.AssignFromSectionValue&&stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.Enum)
if(specialTypeKindisStringParsableTypeKind.Enum)
{
_sourceGenSpec.EmitEnumParseMethod=true;
}
elseif(specialTypeKindis not StringParsableTypeKind.AssignFromSectionValue)
{
_sourceGenSpec.PrimitivesForHelperGen.Add(stringParsableSpec);
}
layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated


Comment on lines

68
 to 
67

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't need this given the suggestion below.
@layomia
layomia
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@pedrobsaila, let's go with checking for the right method in the parser - #89823 (comment). We could also address #89879 in this PR.
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 12, 2023

Closed

3 tasks
@layomialayomia
self-assigned this
Aug 25, 2023
@layomia
layomia
commented
Aug 25, 2023

Copy link


Copy Markdown

Contributor
@pedrobsaila apologies for my delayed response. Could you pls resolve merge conflicts here?
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 27, 2023

Closed

layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 ISet_Unbound = ISet?.ConstructUnboundGenericType();

 // needed to be able to know if a member exist inside the compilation unit
 Enum = compilation.GetBestTypeByMetadataName(typeof(Enum));
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Use the overload taking a SpecialType (i.e. SpecialType.System_Enum). All the special types are guaranteed to be in the input compilation.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 public INamedTypeSymbol? ISet_Unbound { get; }
 public INamedTypeSymbol? ISet { get; }
 public INamedTypeSymbol? List { get; }
 public INamedTypeSymbol? Enum { get; }
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

publicINamedTypeSymbol?Enum{get;}
publicINamedTypeSymbolEnum{get;}
Per https://github.com/dotnet/runtime/pull/89952/files#r1307693295 the fetched value would not be null.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }
 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

_sourceGenSpec.EmitThrowIfNullMethod=!_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod=_typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmptyisfalse;
The symbol could be null; avoid a null-ref exception.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...der/tests/SourceGenerationTests/Baselines/ServiceCollection/Configure_T.net462.generated.txt
@@ -0,0 +1,243 @@
// <auto-generated/>
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for adding the net462 variants. Could we refactor to the following structure?
> baselines
> netcoreapp
> foo.generated.txt
> net462
> foo.generated.txt

I'd actually suggest making a preliminary PR doing this. We'd need to remove the condition on [this line] so that the net462 tests would be included in automated runs. Doing this will reveal a bit of additional, tangential work needed to complete the effort.
@layomia
layomia
commented
Aug 28, 2023

Copy link


Copy Markdown

Contributor
Oof looks like merging #91180 reset your rebase effort. If you take another stab, I'll be sure to check if this PR is merge-ready & queue it up first if so.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs
Outdated

 ExtensionClassType extType = ExtensionClassType.None)
 Action<ImmutableArray<Diagnostic>>? assessDiagnostics = null)
 {
 string path = extType is ExtensionClassType.None
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please use a a preprocessor directive to determine the path
stringpath= #if NETCOREAPP
...
#else ...;
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...soft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.Baselines.cs
Outdated

 {
 string source = GetCollectionsSource();

 await VerifyAgainstBaselineUsingFile("Collections.net462.generated.txt", source, assessDiagnostics: (d) =>
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Per f5a2abc#r1321885499, please revert the changes to these invocations.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please revert back to partitioning the baselines based on the binder ext. type. This is helpful when working issues specific to a particular binder.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
 _sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException is not null && _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty is false;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmpty is false;
Also -- to be doubly sure, check that the method has the expected parameters.
@pedrobsaila
pedrobsaila
commented
Sep 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@layomia can you check it please before the PR become stale again
@eiriktsarpalis
eiriktsarpalis
commented
Oct 4, 2023

Copy link


Copy Markdown

Member
@pedrobsaila apologies for the delayed response, would it be possible to rebase your changes? Thanks!
@tarekgh
tarekgh
commented
Oct 5, 2023•

edited

Loading

Copy link


Copy Markdown

Member
@pedrobsaila do you have a chance to resolve the merging conflicts?
@pedrobsaila
pedrobsaila
commented
Oct 6, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I'm on it. The code in main branch changed deeply, resolving conflict is not easy and will take some time
@pedrobsaila
pedrobsaila
commented
Oct 7, 2023

Copy link


Copy Markdown

ContributorAuthor
@eiriktsarpalis@tarekgh ready for a new round of reviews
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;Baselines\ConfigurationBinder\*.generated.txt;Baselines\OptionsBuilder\*.generated.txt;Baselines\ServiceCollection\*.generated.txt">
 <ItemGroup>
 <Content Include="Baselines\net462\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

"Baselines 
do we need to copy specific files? can't we include the whole Baseline folder?
@pedrobsailapedrobsailaOct 10, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Include just Baselines does not seem to work, the max I could do is this lightweight version :
<Content Include="Baselines\*\*\*.generated.txt;
Baselines\*\*.generated.txt;">

@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You can use the syntax 
runtime/src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Microsoft.Extensions.Options.SourceGeneration.Tests.csproj

Line 42
in
c9b5dd4
 <ContentInclude="Baselines\**\*"> 
@tarekgh
tarekgh
commented
Oct 10, 2023

Copy link


Copy Markdown

Member
@pedrobsaila thanks for getting this ready. I added a few questions if you can help with. otherwise LGTM.
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 Baselines\netcoreapp\ConfigurationBinder\*.generated.txt;
 Baselines\netcoreapp\OptionsBuilder\*.generated.txt;
 Baselines\netcoreapp\ServiceCollection\*.generated.txt;">
 <Content Include="Baselines\*\*\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

this doesn't look right.
@pedrobsailapedrobsailaOct 10, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

seems right to me. There are 2 patterns of files :
  • Baselines\netcoreapp\OptionsBuilder\*.generated.txt => would match Baselines\*\*\*.generated.txt
  • Baselines\netcoreapp\Primitives.generated.txt => would match Baselines\*.generated.txt
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we can copy the whole Baseline as I pointed out in #89952 (comment) that will be better I guess.
tarekgh
tarekgh
approved these changes
Oct 11, 2023
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
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 f1b4930
into
dotnet:mainOct 11, 2023
@pedrobsaila
pedrobsaila
deleted the

89936

branch
October 11, 2023 20:45
@ghostghost
locked as resolved and limited conversation to collaborators
Nov 11, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghtarekgh approved these changes
@eiriktsarpaliseiriktsarpalisAwaiting requested review from eiriktsarpalis
@ericstjericstjAwaiting requested review from ericstj

+1 more reviewer

@layomialayomialayomia left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Extensions-Configurationcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


4 participants

@pedrobsaila@layomia@eiriktsarpalis@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Address feedback from config binding gen PR to improve enum parsing by pedrobsaila · Pull Request #89952 · dotnet/runtime · GitHub
Skip to content
Address feedback from config binding gen PR to improve enum parsing - #89952
Merged
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Oct 11, 2023
Merged
Address feedback from config binding gen PR to improve enum parsing#89952
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Conversation
@pedrobsaila
@pedrobsailapedrobsaila
commented
Aug 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#89936
Fixes#89879
@layomia I addressed the follow-up fixes.
@ghostghost
added

area-Extensions-Configuration

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 3, 2023
@ghost
ghost
commented
Aug 3, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-configuration

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89936
@layomia I addressed 2 of the follow-up fixes. I'll wait for the final decision (polyfill/checking whether Enum.Parse is available) before proceeding to fix it
Author:pedrobsaila
Assignees:-
Labels:
area-Extensions-Configuration
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed

layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'd re-write this as

Suggested change

if(stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.AssignFromSectionValue&&stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.Enum)
if(specialTypeKindisStringParsableTypeKind.Enum)
{
_sourceGenSpec.EmitEnumParseMethod=true;
}
elseif(specialTypeKindis not StringParsableTypeKind.AssignFromSectionValue)
{
_sourceGenSpec.PrimitivesForHelperGen.Add(stringParsableSpec);
}
layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated


Comment on lines

68
 to 
67

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't need this given the suggestion below.
@layomia
layomia
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@pedrobsaila, let's go with checking for the right method in the parser - #89823 (comment). We could also address #89879 in this PR.
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 12, 2023

Closed

3 tasks
@layomialayomia
self-assigned this
Aug 25, 2023
@layomia
layomia
commented
Aug 25, 2023

Copy link


Copy Markdown

Contributor
@pedrobsaila apologies for my delayed response. Could you pls resolve merge conflicts here?
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 27, 2023

Closed

layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 ISet_Unbound = ISet?.ConstructUnboundGenericType();

 // needed to be able to know if a member exist inside the compilation unit
 Enum = compilation.GetBestTypeByMetadataName(typeof(Enum));
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Use the overload taking a SpecialType (i.e. SpecialType.System_Enum). All the special types are guaranteed to be in the input compilation.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 public INamedTypeSymbol? ISet_Unbound { get; }
 public INamedTypeSymbol? ISet { get; }
 public INamedTypeSymbol? List { get; }
 public INamedTypeSymbol? Enum { get; }
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

publicINamedTypeSymbol?Enum{get;}
publicINamedTypeSymbolEnum{get;}
Per https://github.com/dotnet/runtime/pull/89952/files#r1307693295 the fetched value would not be null.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }
 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

_sourceGenSpec.EmitThrowIfNullMethod=!_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod=_typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmptyisfalse;
The symbol could be null; avoid a null-ref exception.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...der/tests/SourceGenerationTests/Baselines/ServiceCollection/Configure_T.net462.generated.txt
@@ -0,0 +1,243 @@
// <auto-generated/>
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for adding the net462 variants. Could we refactor to the following structure?
> baselines
> netcoreapp
> foo.generated.txt
> net462
> foo.generated.txt

I'd actually suggest making a preliminary PR doing this. We'd need to remove the condition on [this line] so that the net462 tests would be included in automated runs. Doing this will reveal a bit of additional, tangential work needed to complete the effort.
@layomia
layomia
commented
Aug 28, 2023

Copy link


Copy Markdown

Contributor
Oof looks like merging #91180 reset your rebase effort. If you take another stab, I'll be sure to check if this PR is merge-ready & queue it up first if so.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs
Outdated

 ExtensionClassType extType = ExtensionClassType.None)
 Action<ImmutableArray<Diagnostic>>? assessDiagnostics = null)
 {
 string path = extType is ExtensionClassType.None
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please use a a preprocessor directive to determine the path
stringpath= #if NETCOREAPP
...
#else ...;
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...soft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.Baselines.cs
Outdated

 {
 string source = GetCollectionsSource();

 await VerifyAgainstBaselineUsingFile("Collections.net462.generated.txt", source, assessDiagnostics: (d) =>
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Per f5a2abc#r1321885499, please revert the changes to these invocations.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please revert back to partitioning the baselines based on the binder ext. type. This is helpful when working issues specific to a particular binder.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
 _sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException is not null && _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty is false;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmpty is false;
Also -- to be doubly sure, check that the method has the expected parameters.
@pedrobsaila
pedrobsaila
commented
Sep 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@layomia can you check it please before the PR become stale again
@eiriktsarpalis
eiriktsarpalis
commented
Oct 4, 2023

Copy link


Copy Markdown

Member
@pedrobsaila apologies for the delayed response, would it be possible to rebase your changes? Thanks!
@tarekgh
tarekgh
commented
Oct 5, 2023•

edited

Loading

Copy link


Copy Markdown

Member
@pedrobsaila do you have a chance to resolve the merging conflicts?
@pedrobsaila
pedrobsaila
commented
Oct 6, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I'm on it. The code in main branch changed deeply, resolving conflict is not easy and will take some time
@pedrobsaila
pedrobsaila
commented
Oct 7, 2023

Copy link


Copy Markdown

ContributorAuthor
@eiriktsarpalis@tarekgh ready for a new round of reviews
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;Baselines\ConfigurationBinder\*.generated.txt;Baselines\OptionsBuilder\*.generated.txt;Baselines\ServiceCollection\*.generated.txt">
 <ItemGroup>
 <Content Include="Baselines\net462\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

"Baselines 
do we need to copy specific files? can't we include the whole Baseline folder?
@pedrobsailapedrobsailaOct 10, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Include just Baselines does not seem to work, the max I could do is this lightweight version :
<Content Include="Baselines\*\*\*.generated.txt;
Baselines\*\*.generated.txt;">

@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You can use the syntax 
runtime/src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Microsoft.Extensions.Options.SourceGeneration.Tests.csproj

Line 42
in
c9b5dd4
 <ContentInclude="Baselines\**\*"> 
@tarekgh
tarekgh
commented
Oct 10, 2023

Copy link


Copy Markdown

Member
@pedrobsaila thanks for getting this ready. I added a few questions if you can help with. otherwise LGTM.
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 Baselines\netcoreapp\ConfigurationBinder\*.generated.txt;
 Baselines\netcoreapp\OptionsBuilder\*.generated.txt;
 Baselines\netcoreapp\ServiceCollection\*.generated.txt;">
 <Content Include="Baselines\*\*\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

this doesn't look right.
@pedrobsailapedrobsailaOct 10, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

seems right to me. There are 2 patterns of files :
  • Baselines\netcoreapp\OptionsBuilder\*.generated.txt => would match Baselines\*\*\*.generated.txt
  • Baselines\netcoreapp\Primitives.generated.txt => would match Baselines\*.generated.txt
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we can copy the whole Baseline as I pointed out in #89952 (comment) that will be better I guess.
tarekgh
tarekgh
approved these changes
Oct 11, 2023
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
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 f1b4930
into
dotnet:mainOct 11, 2023
@pedrobsaila
pedrobsaila
deleted the

89936

branch
October 11, 2023 20:45
@ghostghost
locked as resolved and limited conversation to collaborators
Nov 11, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghtarekgh approved these changes
@eiriktsarpaliseiriktsarpalisAwaiting requested review from eiriktsarpalis
@ericstjericstjAwaiting requested review from ericstj

+1 more reviewer

@layomialayomialayomia left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Extensions-Configurationcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


4 participants

@pedrobsaila@layomia@eiriktsarpalis@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Address feedback from config binding gen PR to improve enum parsing by pedrobsaila · Pull Request #89952 · dotnet/runtime · GitHub
Skip to content
Address feedback from config binding gen PR to improve enum parsing - #89952
Merged
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Oct 11, 2023
Merged
Address feedback from config binding gen PR to improve enum parsing#89952
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Conversation
@pedrobsaila
@pedrobsailapedrobsaila
commented
Aug 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#89936
Fixes#89879
@layomia I addressed the follow-up fixes.
@ghostghost
added

area-Extensions-Configuration

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 3, 2023
@ghost
ghost
commented
Aug 3, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-configuration

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89936
@layomia I addressed 2 of the follow-up fixes. I'll wait for the final decision (polyfill/checking whether Enum.Parse is available) before proceeding to fix it
Author:pedrobsaila
Assignees:-
Labels:
area-Extensions-Configuration
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed

layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'd re-write this as

Suggested change

if(stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.AssignFromSectionValue&&stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.Enum)
if(specialTypeKindisStringParsableTypeKind.Enum)
{
_sourceGenSpec.EmitEnumParseMethod=true;
}
elseif(specialTypeKindis not StringParsableTypeKind.AssignFromSectionValue)
{
_sourceGenSpec.PrimitivesForHelperGen.Add(stringParsableSpec);
}
layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated


Comment on lines

68
 to 
67

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't need this given the suggestion below.
@layomia
layomia
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@pedrobsaila, let's go with checking for the right method in the parser - #89823 (comment). We could also address #89879 in this PR.
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 12, 2023

Closed

3 tasks
@layomialayomia
self-assigned this
Aug 25, 2023
@layomia
layomia
commented
Aug 25, 2023

Copy link


Copy Markdown

Contributor
@pedrobsaila apologies for my delayed response. Could you pls resolve merge conflicts here?
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 27, 2023

Closed

layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 ISet_Unbound = ISet?.ConstructUnboundGenericType();

 // needed to be able to know if a member exist inside the compilation unit
 Enum = compilation.GetBestTypeByMetadataName(typeof(Enum));
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Use the overload taking a SpecialType (i.e. SpecialType.System_Enum). All the special types are guaranteed to be in the input compilation.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 public INamedTypeSymbol? ISet_Unbound { get; }
 public INamedTypeSymbol? ISet { get; }
 public INamedTypeSymbol? List { get; }
 public INamedTypeSymbol? Enum { get; }
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

publicINamedTypeSymbol?Enum{get;}
publicINamedTypeSymbolEnum{get;}
Per https://github.com/dotnet/runtime/pull/89952/files#r1307693295 the fetched value would not be null.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }
 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

_sourceGenSpec.EmitThrowIfNullMethod=!_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod=_typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmptyisfalse;
The symbol could be null; avoid a null-ref exception.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...der/tests/SourceGenerationTests/Baselines/ServiceCollection/Configure_T.net462.generated.txt
@@ -0,0 +1,243 @@
// <auto-generated/>
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for adding the net462 variants. Could we refactor to the following structure?
> baselines
> netcoreapp
> foo.generated.txt
> net462
> foo.generated.txt

I'd actually suggest making a preliminary PR doing this. We'd need to remove the condition on [this line] so that the net462 tests would be included in automated runs. Doing this will reveal a bit of additional, tangential work needed to complete the effort.
@layomia
layomia
commented
Aug 28, 2023

Copy link


Copy Markdown

Contributor
Oof looks like merging #91180 reset your rebase effort. If you take another stab, I'll be sure to check if this PR is merge-ready & queue it up first if so.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs
Outdated

 ExtensionClassType extType = ExtensionClassType.None)
 Action<ImmutableArray<Diagnostic>>? assessDiagnostics = null)
 {
 string path = extType is ExtensionClassType.None
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please use a a preprocessor directive to determine the path
stringpath= #if NETCOREAPP
...
#else ...;
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...soft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.Baselines.cs
Outdated

 {
 string source = GetCollectionsSource();

 await VerifyAgainstBaselineUsingFile("Collections.net462.generated.txt", source, assessDiagnostics: (d) =>
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Per f5a2abc#r1321885499, please revert the changes to these invocations.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please revert back to partitioning the baselines based on the binder ext. type. This is helpful when working issues specific to a particular binder.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
 _sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException is not null && _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty is false;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmpty is false;
Also -- to be doubly sure, check that the method has the expected parameters.
@pedrobsaila
pedrobsaila
commented
Sep 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@layomia can you check it please before the PR become stale again
@eiriktsarpalis
eiriktsarpalis
commented
Oct 4, 2023

Copy link


Copy Markdown

Member
@pedrobsaila apologies for the delayed response, would it be possible to rebase your changes? Thanks!
@tarekgh
tarekgh
commented
Oct 5, 2023•

edited

Loading

Copy link


Copy Markdown

Member
@pedrobsaila do you have a chance to resolve the merging conflicts?
@pedrobsaila
pedrobsaila
commented
Oct 6, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I'm on it. The code in main branch changed deeply, resolving conflict is not easy and will take some time
@pedrobsaila
pedrobsaila
commented
Oct 7, 2023

Copy link


Copy Markdown

ContributorAuthor
@eiriktsarpalis@tarekgh ready for a new round of reviews
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;Baselines\ConfigurationBinder\*.generated.txt;Baselines\OptionsBuilder\*.generated.txt;Baselines\ServiceCollection\*.generated.txt">
 <ItemGroup>
 <Content Include="Baselines\net462\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

"Baselines 
do we need to copy specific files? can't we include the whole Baseline folder?
@pedrobsailapedrobsailaOct 10, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Include just Baselines does not seem to work, the max I could do is this lightweight version :
<Content Include="Baselines\*\*\*.generated.txt;
Baselines\*\*.generated.txt;">

@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You can use the syntax 
runtime/src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Microsoft.Extensions.Options.SourceGeneration.Tests.csproj

Line 42
in
c9b5dd4
 <ContentInclude="Baselines\**\*"> 
@tarekgh
tarekgh
commented
Oct 10, 2023

Copy link


Copy Markdown

Member
@pedrobsaila thanks for getting this ready. I added a few questions if you can help with. otherwise LGTM.
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 Baselines\netcoreapp\ConfigurationBinder\*.generated.txt;
 Baselines\netcoreapp\OptionsBuilder\*.generated.txt;
 Baselines\netcoreapp\ServiceCollection\*.generated.txt;">
 <Content Include="Baselines\*\*\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

this doesn't look right.
@pedrobsailapedrobsailaOct 10, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

seems right to me. There are 2 patterns of files :
  • Baselines\netcoreapp\OptionsBuilder\*.generated.txt => would match Baselines\*\*\*.generated.txt
  • Baselines\netcoreapp\Primitives.generated.txt => would match Baselines\*.generated.txt
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we can copy the whole Baseline as I pointed out in #89952 (comment) that will be better I guess.
tarekgh
tarekgh
approved these changes
Oct 11, 2023
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
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 f1b4930
into
dotnet:mainOct 11, 2023
@pedrobsaila
pedrobsaila
deleted the

89936

branch
October 11, 2023 20:45
@ghostghost
locked as resolved and limited conversation to collaborators
Nov 11, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghtarekgh approved these changes
@eiriktsarpaliseiriktsarpalisAwaiting requested review from eiriktsarpalis
@ericstjericstjAwaiting requested review from ericstj

+1 more reviewer

@layomialayomialayomia left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Extensions-Configurationcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


4 participants

@pedrobsaila@layomia@eiriktsarpalis@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Address feedback from config binding gen PR to improve enum parsing by pedrobsaila · Pull Request #89952 · dotnet/runtime · GitHub
Skip to content
Address feedback from config binding gen PR to improve enum parsing - #89952
Merged
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Oct 11, 2023
Merged
Address feedback from config binding gen PR to improve enum parsing#89952
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Conversation
@pedrobsaila
@pedrobsailapedrobsaila
commented
Aug 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#89936
Fixes#89879
@layomia I addressed the follow-up fixes.
@ghostghost
added

area-Extensions-Configuration

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 3, 2023
@ghost
ghost
commented
Aug 3, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-configuration

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89936
@layomia I addressed 2 of the follow-up fixes. I'll wait for the final decision (polyfill/checking whether Enum.Parse is available) before proceeding to fix it
Author:pedrobsaila
Assignees:-
Labels:
area-Extensions-Configuration
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed

layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'd re-write this as

Suggested change

if(stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.AssignFromSectionValue&&stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.Enum)
if(specialTypeKindisStringParsableTypeKind.Enum)
{
_sourceGenSpec.EmitEnumParseMethod=true;
}
elseif(specialTypeKindis not StringParsableTypeKind.AssignFromSectionValue)
{
_sourceGenSpec.PrimitivesForHelperGen.Add(stringParsableSpec);
}
layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated


Comment on lines

68
 to 
67

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't need this given the suggestion below.
@layomia
layomia
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@pedrobsaila, let's go with checking for the right method in the parser - #89823 (comment). We could also address #89879 in this PR.
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 12, 2023

Closed

3 tasks
@layomialayomia
self-assigned this
Aug 25, 2023
@layomia
layomia
commented
Aug 25, 2023

Copy link


Copy Markdown

Contributor
@pedrobsaila apologies for my delayed response. Could you pls resolve merge conflicts here?
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 27, 2023

Closed

layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 ISet_Unbound = ISet?.ConstructUnboundGenericType();

 // needed to be able to know if a member exist inside the compilation unit
 Enum = compilation.GetBestTypeByMetadataName(typeof(Enum));
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Use the overload taking a SpecialType (i.e. SpecialType.System_Enum). All the special types are guaranteed to be in the input compilation.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 public INamedTypeSymbol? ISet_Unbound { get; }
 public INamedTypeSymbol? ISet { get; }
 public INamedTypeSymbol? List { get; }
 public INamedTypeSymbol? Enum { get; }
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

publicINamedTypeSymbol?Enum{get;}
publicINamedTypeSymbolEnum{get;}
Per https://github.com/dotnet/runtime/pull/89952/files#r1307693295 the fetched value would not be null.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }
 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

_sourceGenSpec.EmitThrowIfNullMethod=!_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod=_typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmptyisfalse;
The symbol could be null; avoid a null-ref exception.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...der/tests/SourceGenerationTests/Baselines/ServiceCollection/Configure_T.net462.generated.txt
@@ -0,0 +1,243 @@
// <auto-generated/>
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for adding the net462 variants. Could we refactor to the following structure?
> baselines
> netcoreapp
> foo.generated.txt
> net462
> foo.generated.txt

I'd actually suggest making a preliminary PR doing this. We'd need to remove the condition on [this line] so that the net462 tests would be included in automated runs. Doing this will reveal a bit of additional, tangential work needed to complete the effort.
@layomia
layomia
commented
Aug 28, 2023

Copy link


Copy Markdown

Contributor
Oof looks like merging #91180 reset your rebase effort. If you take another stab, I'll be sure to check if this PR is merge-ready & queue it up first if so.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs
Outdated

 ExtensionClassType extType = ExtensionClassType.None)
 Action<ImmutableArray<Diagnostic>>? assessDiagnostics = null)
 {
 string path = extType is ExtensionClassType.None
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please use a a preprocessor directive to determine the path
stringpath= #if NETCOREAPP
...
#else ...;
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...soft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.Baselines.cs
Outdated

 {
 string source = GetCollectionsSource();

 await VerifyAgainstBaselineUsingFile("Collections.net462.generated.txt", source, assessDiagnostics: (d) =>
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Per f5a2abc#r1321885499, please revert the changes to these invocations.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please revert back to partitioning the baselines based on the binder ext. type. This is helpful when working issues specific to a particular binder.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
 _sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException is not null && _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty is false;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmpty is false;
Also -- to be doubly sure, check that the method has the expected parameters.
@pedrobsaila
pedrobsaila
commented
Sep 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@layomia can you check it please before the PR become stale again
@eiriktsarpalis
eiriktsarpalis
commented
Oct 4, 2023

Copy link


Copy Markdown

Member
@pedrobsaila apologies for the delayed response, would it be possible to rebase your changes? Thanks!
@tarekgh
tarekgh
commented
Oct 5, 2023•

edited

Loading

Copy link


Copy Markdown

Member
@pedrobsaila do you have a chance to resolve the merging conflicts?
@pedrobsaila
pedrobsaila
commented
Oct 6, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I'm on it. The code in main branch changed deeply, resolving conflict is not easy and will take some time
@pedrobsaila
pedrobsaila
commented
Oct 7, 2023

Copy link


Copy Markdown

ContributorAuthor
@eiriktsarpalis@tarekgh ready for a new round of reviews
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;Baselines\ConfigurationBinder\*.generated.txt;Baselines\OptionsBuilder\*.generated.txt;Baselines\ServiceCollection\*.generated.txt">
 <ItemGroup>
 <Content Include="Baselines\net462\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

"Baselines 
do we need to copy specific files? can't we include the whole Baseline folder?
@pedrobsailapedrobsailaOct 10, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Include just Baselines does not seem to work, the max I could do is this lightweight version :
<Content Include="Baselines\*\*\*.generated.txt;
Baselines\*\*.generated.txt;">

@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You can use the syntax 
runtime/src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Microsoft.Extensions.Options.SourceGeneration.Tests.csproj

Line 42
in
c9b5dd4
 <ContentInclude="Baselines\**\*"> 
@tarekgh
tarekgh
commented
Oct 10, 2023

Copy link


Copy Markdown

Member
@pedrobsaila thanks for getting this ready. I added a few questions if you can help with. otherwise LGTM.
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 Baselines\netcoreapp\ConfigurationBinder\*.generated.txt;
 Baselines\netcoreapp\OptionsBuilder\*.generated.txt;
 Baselines\netcoreapp\ServiceCollection\*.generated.txt;">
 <Content Include="Baselines\*\*\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

this doesn't look right.
@pedrobsailapedrobsailaOct 10, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

seems right to me. There are 2 patterns of files :
  • Baselines\netcoreapp\OptionsBuilder\*.generated.txt => would match Baselines\*\*\*.generated.txt
  • Baselines\netcoreapp\Primitives.generated.txt => would match Baselines\*.generated.txt
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we can copy the whole Baseline as I pointed out in #89952 (comment) that will be better I guess.
tarekgh
tarekgh
approved these changes
Oct 11, 2023
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
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 f1b4930
into
dotnet:mainOct 11, 2023
@pedrobsaila
pedrobsaila
deleted the

89936

branch
October 11, 2023 20:45
@ghostghost
locked as resolved and limited conversation to collaborators
Nov 11, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghtarekgh approved these changes
@eiriktsarpaliseiriktsarpalisAwaiting requested review from eiriktsarpalis
@ericstjericstjAwaiting requested review from ericstj

+1 more reviewer

@layomialayomialayomia left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Extensions-Configurationcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


4 participants

@pedrobsaila@layomia@eiriktsarpalis@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
Address feedback from config binding gen PR to improve enum parsing by pedrobsaila · Pull Request #89952 · dotnet/runtime · GitHub
Skip to content
Address feedback from config binding gen PR to improve enum parsing - #89952
Merged
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Oct 11, 2023
Merged
Address feedback from config binding gen PR to improve enum parsing#89952
tarekgh merged 22 commits into
dotnet:mainfrom 
pedrobsaila:89936
Conversation
@pedrobsaila
@pedrobsailapedrobsaila
commented
Aug 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#89936
Fixes#89879
@layomia I addressed the follow-up fixes.
@ghostghost
added

area-Extensions-Configuration

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 3, 2023
@ghost
ghost
commented
Aug 3, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-configuration

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89936
@layomia I addressed 2 of the follow-up fixes. I'll wait for the final decision (polyfill/checking whether Enum.Parse is available) before proceeding to fix it
Author:pedrobsaila
Assignees:-
Labels:
area-Extensions-Configuration
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed

layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'd re-write this as

Suggested change

if(stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.AssignFromSectionValue&&stringParsableSpec.StringParsableTypeKind!=StringParsableTypeKind.Enum)
if(specialTypeKindisStringParsableTypeKind.Enum)
{
_sourceGenSpec.EmitEnumParseMethod=true;
}
elseif(specialTypeKindis not StringParsableTypeKind.AssignFromSectionValue)
{
_sourceGenSpec.PrimitivesForHelperGen.Add(stringParsableSpec);
}
layomia
layomia
reviewed
Aug 9, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated


Comment on lines

68
 to 
67

@layomialayomiaAug 9, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't need this given the suggestion below.
@layomia
layomia
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@pedrobsaila, let's go with checking for the right method in the parser - #89823 (comment). We could also address #89879 in this PR.
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 12, 2023

Closed

3 tasks
@layomialayomia
self-assigned this
Aug 25, 2023
@layomia
layomia
commented
Aug 25, 2023

Copy link


Copy Markdown

Contributor
@pedrobsaila apologies for my delayed response. Could you pls resolve merge conflicts here?
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 27, 2023

Closed

layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 ISet_Unbound = ISet?.ConstructUnboundGenericType();

 // needed to be able to know if a member exist inside the compilation unit
 Enum = compilation.GetBestTypeByMetadataName(typeof(Enum));
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Use the overload taking a SpecialType (i.e. SpecialType.System_Enum). All the special types are guaranteed to be in the input compilation.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs
Outdated

 public INamedTypeSymbol? ISet_Unbound { get; }
 public INamedTypeSymbol? ISet { get; }
 public INamedTypeSymbol? List { get; }
 public INamedTypeSymbol? Enum { get; }
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

publicINamedTypeSymbol?Enum{get;}
publicINamedTypeSymbolEnum{get;}
Per https://github.com/dotnet/runtime/pull/89952/files#r1307693295 the fetched value would not be null.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }
 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

_sourceGenSpec.EmitThrowIfNullMethod=!_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod=_typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmptyisfalse;
The symbol could be null; avoid a null-ref exception.
layomia
layomia
reviewed
Aug 28, 2023
View reviewed changes
Comment thread...der/tests/SourceGenerationTests/Baselines/ServiceCollection/Configure_T.net462.generated.txt
@@ -0,0 +1,243 @@
// <auto-generated/>
@layomialayomiaAug 28, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for adding the net462 variants. Could we refactor to the following structure?
> baselines
> netcoreapp
> foo.generated.txt
> net462
> foo.generated.txt

I'd actually suggest making a preliminary PR doing this. We'd need to remove the condition on [this line] so that the net462 tests would be included in automated runs. Doing this will reveal a bit of additional, tangential work needed to complete the effort.
@layomia
layomia
commented
Aug 28, 2023

Copy link


Copy Markdown

Contributor
Oof looks like merging #91180 reset your rebase effort. If you take another stab, I'll be sure to check if this PR is merge-ready & queue it up first if so.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs
Outdated

 ExtensionClassType extType = ExtensionClassType.None)
 Action<ImmutableArray<Diagnostic>>? assessDiagnostics = null)
 {
 string path = extType is ExtensionClassType.None
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please use a a preprocessor directive to determine the path
stringpath= #if NETCOREAPP
...
#else ...;
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...soft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.Baselines.cs
Outdated

 {
 string source = GetCollectionsSource();

 await VerifyAgainstBaselineUsingFile("Collections.net462.generated.txt", source, assessDiagnostics: (d) =>
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Per f5a2abc#r1321885499, please revert the changes to these invocations.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please revert back to partitioning the baselines based on the binder ext. type. This is helpful when working issues specific to a particular binder.
layomia
layomia
reviewed
Sep 11, 2023
View reviewed changes
Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated

 }

 _sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
 _sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException is not null && _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty is false;
@layomialayomiaSep 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmpty is false;
Also -- to be doubly sure, check that the method has the expected parameters.
@pedrobsaila
pedrobsaila
commented
Sep 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@layomia can you check it please before the PR become stale again
@eiriktsarpalis
eiriktsarpalis
commented
Oct 4, 2023

Copy link


Copy Markdown

Member
@pedrobsaila apologies for the delayed response, would it be possible to rebase your changes? Thanks!
@tarekgh
tarekgh
commented
Oct 5, 2023•

edited

Loading

Copy link


Copy Markdown

Member
@pedrobsaila do you have a chance to resolve the merging conflicts?
@pedrobsaila
pedrobsaila
commented
Oct 6, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I'm on it. The code in main branch changed deeply, resolving conflict is not easy and will take some time
@pedrobsaila
pedrobsaila
commented
Oct 7, 2023

Copy link


Copy Markdown

ContributorAuthor
@eiriktsarpalis@tarekgh ready for a new round of reviews
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
 <Content Include="Baselines\*.generated.txt;Baselines\ConfigurationBinder\*.generated.txt;Baselines\OptionsBuilder\*.generated.txt;Baselines\ServiceCollection\*.generated.txt">
 <ItemGroup>
 <Content Include="Baselines\net462\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

"Baselines 
do we need to copy specific files? can't we include the whole Baseline folder?
@pedrobsailapedrobsailaOct 10, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Include just Baselines does not seem to work, the max I could do is this lightweight version :
<Content Include="Baselines\*\*\*.generated.txt;
Baselines\*\*.generated.txt;">

@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You can use the syntax 
runtime/src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Microsoft.Extensions.Options.SourceGeneration.Tests.csproj

Line 42
in
c9b5dd4
 <ContentInclude="Baselines\**\*"> 
@tarekgh
tarekgh
commented
Oct 10, 2023

Copy link


Copy Markdown

Member
@pedrobsaila thanks for getting this ready. I added a few questions if you can help with. otherwise LGTM.
tarekgh
tarekgh
reviewed
Oct 10, 2023
View reviewed changes
Comment thread...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj
Outdated

 Baselines\netcoreapp\ConfigurationBinder\*.generated.txt;
 Baselines\netcoreapp\OptionsBuilder\*.generated.txt;
 Baselines\netcoreapp\ServiceCollection\*.generated.txt;">
 <Content Include="Baselines\*\*\*.generated.txt;
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

this doesn't look right.
@pedrobsailapedrobsailaOct 10, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

seems right to me. There are 2 patterns of files :
  • Baselines\netcoreapp\OptionsBuilder\*.generated.txt => would match Baselines\*\*\*.generated.txt
  • Baselines\netcoreapp\Primitives.generated.txt => would match Baselines\*.generated.txt
@tarekghtarekghOct 10, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we can copy the whole Baseline as I pointed out in #89952 (comment) that will be better I guess.
tarekgh
tarekgh
approved these changes
Oct 11, 2023
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
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 f1b4930
into
dotnet:mainOct 11, 2023
@pedrobsaila
pedrobsaila
deleted the

89936

branch
October 11, 2023 20:45
@ghostghost
locked as resolved and limited conversation to collaborators
Nov 11, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghtarekgh approved these changes
@eiriktsarpaliseiriktsarpalisAwaiting requested review from eiriktsarpalis
@ericstjericstjAwaiting requested review from ericstj

+1 more reviewer

@layomialayomialayomia left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Extensions-Configurationcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


4 participants

@pedrobsaila@layomia@eiriktsarpalis@tarekgh