Skip to content

Make src gen for property setters consistent with reflection - #91899

Merged
steveharter merged 6 commits into
dotnet:mainfrom
steveharter:GenPropertySet
Sep 14, 2023
Merged

Make src gen for property setters consistent with reflection#91899
steveharter merged 6 commits into
dotnet:mainfrom
steveharter:GenPropertySet

Conversation

@steveharter

@stevehartersteveharter commented Sep 11, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Address #86333 (RC2 candidate as well)

  • Call the setter for value type properties (with the default value for the property type) if the value is not in the config, unless it is during a Bind (not "Get"). This requires a adding a bool arg to the BindCore() method.
  • Always check for a null value or missing section for properties and if null\missing (this was not done in all cases previously) then don't call the setter with the null. This addresses Config generator overwrites existing properties when provided with empty config section #91380.

Note that this PR matches known semantics regarding the above with the goal of maintaining compat with reflection; this is the case even for non-intuitive cases including:

  • Only value type properties get their setter called; not reference types or Nullable<T>.
  • Also, properties on objects that are nested objects or those added to a collection are also not defaulted through the setter (only properties on root objects are).

We could create an issue for these non-intuitive cases, but would need to consider the benefit:risk including breaking changes.

@stevehartersteveharter self-assigned this Sep 11, 2023
@ghost

ghost commented Sep 11, 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

null

Author:steveharter
Assignees:steveharter
Labels:

area-Extensions-Configuration

Milestone:-

@tarekgh

tarekgh commented Sep 12, 2023

Copy link
Copy Markdown
Member

@steveharter there are some failures that seem related to the change. Could you please have a look?

layomia
layomia reviewed Sep 12, 2023
View reviewed changes
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt Outdated
Comment thread...nfiguration.Binder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T.generated.txt
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
@stevehartersteveharter added the source-generator Indicates an issue with a source generator feature label Sep 13, 2023
@steveharter
steveharter marked this pull request as ready for review September 13, 2023 01:56
layomia
layomia reviewed Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs Outdated
layomia
layomia reviewed Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs Outdated
@stevehartersteveharter modified the milestone: 8.0.0Sep 14, 2023
layomia
layomia approved these changes Sep 14, 2023
View reviewed changes

@layomialayomia left a comment

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!

@steveharter
steveharter merged commit 650eec9 into dotnet:mainSep 14, 2023
@steveharter
steveharter deleted the GenPropertySet branch September 14, 2023 21:33
@steveharter

steveharter commented Sep 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Need to get in at least this before backport merge can be successful: #91717

@steveharter

steveharter commented Sep 14, 2023

Copy link
Copy Markdown
ContributorAuthor

/backport to release/8.0

@github-actions

github-actionsBot commented Sep 14, 2023

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6191249484

@github-actions

github-actionsBot commented Sep 14, 2023

Copy link
Copy Markdown
Contributor

@steveharter backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Make src gen for property setters consistent with reflection
.git/rebase-apply/patch:218: trailing whitespace.
Assert.Null(options.OtherCodeUri); warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/Types/SimpleTypeSpec.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/ParsableFromStringSpec.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make src gen for property setters consistent with reflection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions

github-actionsBot commented Sep 14, 2023

Copy link
Copy Markdown
Contributor

@steveharter an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

@stevehartersteveharter added this to the 8.0.0 milestone Sep 14, 2023
@stevehartersteveharter mentioned this pull request Sep 15, 2023
Merged
@steveharter
steveharter restored the GenPropertySet branch September 15, 2023 21:42
@steveharter
steveharter deleted the GenPropertySet branch September 15, 2023 21:45
@ericstj

ericstj commented Sep 16, 2023

Copy link
Copy Markdown
Member

/backport to release/8.0

@github-actions

github-actionsBot commented Sep 16, 2023

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6204180109

@github-actionsgithub-actionsBot mentioned this pull request Sep 16, 2023
Merged
@ghostghost locked as resolved and limited conversation to collaborators Oct 16, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tarekghtarekghAwaiting requested review from tarekgh
1 more reviewer
@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-Configurationsource-generatorIndicates an issue with a source generator feature

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@steveharter@tarekgh@ericstj@layomia
, '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" + '
Make src gen for property setters consistent with reflection by steveharter · Pull Request #91899 · dotnet/runtime · GitHub
Skip to content
Make src gen for property setters consistent with reflection - #91899
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Sep 14, 2023
Merged
Make src gen for property setters consistent with reflection#91899
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Conversation
@steveharter
@stevehartersteveharter
commented
Sep 11, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Address #86333 (RC2 candidate as well)
  • Call the setter for value type properties (with the default value for the property type) if the value is not in the config, unless it is during a Bind (not "Get"). This requires a adding a bool arg to the BindCore() method.
  • Always check for a null value or missing section for properties and if null\missing (this was not done in all cases previously) then don't call the setter with the null. This addresses Config generator overwrites existing properties when provided with empty config section #91380.
Note that this PR matches known semantics regarding the above with the goal of maintaining compat with reflection; this is the case even for non-intuitive cases including:
  • Only value type properties get their setter called; not reference types or Nullable<T>.
  • Also, properties on objects that are nested objects or those added to a collection are also not defaulted through the setter (only properties on root objects are).
We could create an issue for these non-intuitive cases, but would need to consider the benefit:risk including breaking changes.
@stevehartersteveharter
self-assigned this
Sep 11, 2023
@ghost
ghost
commented
Sep 11, 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
null
Author:steveharter
Assignees:steveharter
Labels:
area-Extensions-Configuration
Milestone:-
@tarekgh
tarekgh
commented
Sep 12, 2023

Copy link


Copy Markdown

Member
@steveharter there are some failures that seem related to the change. Could you please have a look?
layomia
layomia
reviewed
Sep 12, 2023
View reviewed changes
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
Outdated

Comment thread...nfiguration.Binder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T.generated.txt
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
@stevehartersteveharter
added
the 
source-generator
Indicates an issue with a source generator feature
label
Sep 13, 2023
@steveharter
steveharter
marked this pull request as ready for review
September 13, 2023 01:56
layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

@stevehartersteveharter
modified the milestone:
8.0.0Sep 14, 2023
layomia
layomia
approved these changes
Sep 14, 2023
View reviewed changes
@layomialayomia
left a comment


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!
@steveharter
steveharter
merged commit 650eec9
into
dotnet:mainSep 14, 2023
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 14, 2023 21:33
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Need to get in at least this before backport merge can be successful: #91717
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6191249484
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter backporting to release/8.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Make src gen for property setters consistent with reflection
.git/rebase-apply/patch:218: trailing whitespace.
Assert.Null(options.OtherCodeUri); warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/Types/SimpleTypeSpec.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/ParsableFromStringSpec.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make src gen for property setters consistent with reflection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter an error occurred while backporting to release/8.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@stevehartersteveharter
added this to the 8.0.0 milestone
Sep 14, 2023
@stevehartersteveharter
mentioned this pull request
Sep 15, 2023
 Merged

@steveharter
steveharter
restored the

GenPropertySet

branch
September 15, 2023 21:42
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 15, 2023 21:45
@ericstj
ericstj
commented
Sep 16, 2023

Copy link


Copy Markdown

Member
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 16, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6204180109
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 16, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 16, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghAwaiting requested review from tarekgh

1 more reviewer

@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-Configurationsource-generatorIndicates an issue with a source generator feature

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@steveharter@tarekgh@ericstj@layomia

, '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('^' + ".*" + '
Make src gen for property setters consistent with reflection by steveharter · Pull Request #91899 · dotnet/runtime · GitHub
Skip to content
Make src gen for property setters consistent with reflection - #91899
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Sep 14, 2023
Merged
Make src gen for property setters consistent with reflection#91899
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Conversation
@steveharter
@stevehartersteveharter
commented
Sep 11, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Address #86333 (RC2 candidate as well)
  • Call the setter for value type properties (with the default value for the property type) if the value is not in the config, unless it is during a Bind (not "Get"). This requires a adding a bool arg to the BindCore() method.
  • Always check for a null value or missing section for properties and if null\missing (this was not done in all cases previously) then don't call the setter with the null. This addresses Config generator overwrites existing properties when provided with empty config section #91380.
Note that this PR matches known semantics regarding the above with the goal of maintaining compat with reflection; this is the case even for non-intuitive cases including:
  • Only value type properties get their setter called; not reference types or Nullable<T>.
  • Also, properties on objects that are nested objects or those added to a collection are also not defaulted through the setter (only properties on root objects are).
We could create an issue for these non-intuitive cases, but would need to consider the benefit:risk including breaking changes.
@stevehartersteveharter
self-assigned this
Sep 11, 2023
@ghost
ghost
commented
Sep 11, 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
null
Author:steveharter
Assignees:steveharter
Labels:
area-Extensions-Configuration
Milestone:-
@tarekgh
tarekgh
commented
Sep 12, 2023

Copy link


Copy Markdown

Member
@steveharter there are some failures that seem related to the change. Could you please have a look?
layomia
layomia
reviewed
Sep 12, 2023
View reviewed changes
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
Outdated

Comment thread...nfiguration.Binder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T.generated.txt
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
@stevehartersteveharter
added
the 
source-generator
Indicates an issue with a source generator feature
label
Sep 13, 2023
@steveharter
steveharter
marked this pull request as ready for review
September 13, 2023 01:56
layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

@stevehartersteveharter
modified the milestone:
8.0.0Sep 14, 2023
layomia
layomia
approved these changes
Sep 14, 2023
View reviewed changes
@layomialayomia
left a comment


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!
@steveharter
steveharter
merged commit 650eec9
into
dotnet:mainSep 14, 2023
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 14, 2023 21:33
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Need to get in at least this before backport merge can be successful: #91717
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6191249484
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter backporting to release/8.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Make src gen for property setters consistent with reflection
.git/rebase-apply/patch:218: trailing whitespace.
Assert.Null(options.OtherCodeUri); warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/Types/SimpleTypeSpec.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/ParsableFromStringSpec.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make src gen for property setters consistent with reflection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter an error occurred while backporting to release/8.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@stevehartersteveharter
added this to the 8.0.0 milestone
Sep 14, 2023
@stevehartersteveharter
mentioned this pull request
Sep 15, 2023
 Merged

@steveharter
steveharter
restored the

GenPropertySet

branch
September 15, 2023 21:42
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 15, 2023 21:45
@ericstj
ericstj
commented
Sep 16, 2023

Copy link


Copy Markdown

Member
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 16, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6204180109
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 16, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 16, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghAwaiting requested review from tarekgh

1 more reviewer

@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-Configurationsource-generatorIndicates an issue with a source generator feature

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@steveharter@tarekgh@ericstj@layomia

, '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('^' + ".*" + '
Make src gen for property setters consistent with reflection by steveharter · Pull Request #91899 · dotnet/runtime · GitHub
Skip to content
Make src gen for property setters consistent with reflection - #91899
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Sep 14, 2023
Merged
Make src gen for property setters consistent with reflection#91899
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Conversation
@steveharter
@stevehartersteveharter
commented
Sep 11, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Address #86333 (RC2 candidate as well)
  • Call the setter for value type properties (with the default value for the property type) if the value is not in the config, unless it is during a Bind (not "Get"). This requires a adding a bool arg to the BindCore() method.
  • Always check for a null value or missing section for properties and if null\missing (this was not done in all cases previously) then don't call the setter with the null. This addresses Config generator overwrites existing properties when provided with empty config section #91380.
Note that this PR matches known semantics regarding the above with the goal of maintaining compat with reflection; this is the case even for non-intuitive cases including:
  • Only value type properties get their setter called; not reference types or Nullable<T>.
  • Also, properties on objects that are nested objects or those added to a collection are also not defaulted through the setter (only properties on root objects are).
We could create an issue for these non-intuitive cases, but would need to consider the benefit:risk including breaking changes.
@stevehartersteveharter
self-assigned this
Sep 11, 2023
@ghost
ghost
commented
Sep 11, 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
null
Author:steveharter
Assignees:steveharter
Labels:
area-Extensions-Configuration
Milestone:-
@tarekgh
tarekgh
commented
Sep 12, 2023

Copy link


Copy Markdown

Member
@steveharter there are some failures that seem related to the change. Could you please have a look?
layomia
layomia
reviewed
Sep 12, 2023
View reviewed changes
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
Outdated

Comment thread...nfiguration.Binder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T.generated.txt
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
@stevehartersteveharter
added
the 
source-generator
Indicates an issue with a source generator feature
label
Sep 13, 2023
@steveharter
steveharter
marked this pull request as ready for review
September 13, 2023 01:56
layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

@stevehartersteveharter
modified the milestone:
8.0.0Sep 14, 2023
layomia
layomia
approved these changes
Sep 14, 2023
View reviewed changes
@layomialayomia
left a comment


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!
@steveharter
steveharter
merged commit 650eec9
into
dotnet:mainSep 14, 2023
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 14, 2023 21:33
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Need to get in at least this before backport merge can be successful: #91717
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6191249484
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter backporting to release/8.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Make src gen for property setters consistent with reflection
.git/rebase-apply/patch:218: trailing whitespace.
Assert.Null(options.OtherCodeUri); warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/Types/SimpleTypeSpec.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/ParsableFromStringSpec.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make src gen for property setters consistent with reflection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter an error occurred while backporting to release/8.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@stevehartersteveharter
added this to the 8.0.0 milestone
Sep 14, 2023
@stevehartersteveharter
mentioned this pull request
Sep 15, 2023
 Merged

@steveharter
steveharter
restored the

GenPropertySet

branch
September 15, 2023 21:42
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 15, 2023 21:45
@ericstj
ericstj
commented
Sep 16, 2023

Copy link


Copy Markdown

Member
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 16, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6204180109
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 16, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 16, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghAwaiting requested review from tarekgh

1 more reviewer

@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-Configurationsource-generatorIndicates an issue with a source generator feature

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@steveharter@tarekgh@ericstj@layomia

, '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" + '
Make src gen for property setters consistent with reflection by steveharter · Pull Request #91899 · dotnet/runtime · GitHub
Skip to content
Make src gen for property setters consistent with reflection - #91899
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Sep 14, 2023
Merged
Make src gen for property setters consistent with reflection#91899
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Conversation
@steveharter
@stevehartersteveharter
commented
Sep 11, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Address #86333 (RC2 candidate as well)
  • Call the setter for value type properties (with the default value for the property type) if the value is not in the config, unless it is during a Bind (not "Get"). This requires a adding a bool arg to the BindCore() method.
  • Always check for a null value or missing section for properties and if null\missing (this was not done in all cases previously) then don't call the setter with the null. This addresses Config generator overwrites existing properties when provided with empty config section #91380.
Note that this PR matches known semantics regarding the above with the goal of maintaining compat with reflection; this is the case even for non-intuitive cases including:
  • Only value type properties get their setter called; not reference types or Nullable<T>.
  • Also, properties on objects that are nested objects or those added to a collection are also not defaulted through the setter (only properties on root objects are).
We could create an issue for these non-intuitive cases, but would need to consider the benefit:risk including breaking changes.
@stevehartersteveharter
self-assigned this
Sep 11, 2023
@ghost
ghost
commented
Sep 11, 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
null
Author:steveharter
Assignees:steveharter
Labels:
area-Extensions-Configuration
Milestone:-
@tarekgh
tarekgh
commented
Sep 12, 2023

Copy link


Copy Markdown

Member
@steveharter there are some failures that seem related to the change. Could you please have a look?
layomia
layomia
reviewed
Sep 12, 2023
View reviewed changes
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
Outdated

Comment thread...nfiguration.Binder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T.generated.txt
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
@stevehartersteveharter
added
the 
source-generator
Indicates an issue with a source generator feature
label
Sep 13, 2023
@steveharter
steveharter
marked this pull request as ready for review
September 13, 2023 01:56
layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

@stevehartersteveharter
modified the milestone:
8.0.0Sep 14, 2023
layomia
layomia
approved these changes
Sep 14, 2023
View reviewed changes
@layomialayomia
left a comment


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!
@steveharter
steveharter
merged commit 650eec9
into
dotnet:mainSep 14, 2023
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 14, 2023 21:33
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Need to get in at least this before backport merge can be successful: #91717
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6191249484
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter backporting to release/8.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Make src gen for property setters consistent with reflection
.git/rebase-apply/patch:218: trailing whitespace.
Assert.Null(options.OtherCodeUri); warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/Types/SimpleTypeSpec.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/ParsableFromStringSpec.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make src gen for property setters consistent with reflection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter an error occurred while backporting to release/8.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@stevehartersteveharter
added this to the 8.0.0 milestone
Sep 14, 2023
@stevehartersteveharter
mentioned this pull request
Sep 15, 2023
 Merged

@steveharter
steveharter
restored the

GenPropertySet

branch
September 15, 2023 21:42
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 15, 2023 21:45
@ericstj
ericstj
commented
Sep 16, 2023

Copy link


Copy Markdown

Member
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 16, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6204180109
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 16, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 16, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghAwaiting requested review from tarekgh

1 more reviewer

@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-Configurationsource-generatorIndicates an issue with a source generator feature

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@steveharter@tarekgh@ericstj@layomia

, '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('^' + ".*" + '
Make src gen for property setters consistent with reflection by steveharter · Pull Request #91899 · dotnet/runtime · GitHub
Skip to content
Make src gen for property setters consistent with reflection - #91899
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Sep 14, 2023
Merged
Make src gen for property setters consistent with reflection#91899
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Conversation
@steveharter
@stevehartersteveharter
commented
Sep 11, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Address #86333 (RC2 candidate as well)
  • Call the setter for value type properties (with the default value for the property type) if the value is not in the config, unless it is during a Bind (not "Get"). This requires a adding a bool arg to the BindCore() method.
  • Always check for a null value or missing section for properties and if null\missing (this was not done in all cases previously) then don't call the setter with the null. This addresses Config generator overwrites existing properties when provided with empty config section #91380.
Note that this PR matches known semantics regarding the above with the goal of maintaining compat with reflection; this is the case even for non-intuitive cases including:
  • Only value type properties get their setter called; not reference types or Nullable<T>.
  • Also, properties on objects that are nested objects or those added to a collection are also not defaulted through the setter (only properties on root objects are).
We could create an issue for these non-intuitive cases, but would need to consider the benefit:risk including breaking changes.
@stevehartersteveharter
self-assigned this
Sep 11, 2023
@ghost
ghost
commented
Sep 11, 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
null
Author:steveharter
Assignees:steveharter
Labels:
area-Extensions-Configuration
Milestone:-
@tarekgh
tarekgh
commented
Sep 12, 2023

Copy link


Copy Markdown

Member
@steveharter there are some failures that seem related to the change. Could you please have a look?
layomia
layomia
reviewed
Sep 12, 2023
View reviewed changes
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
Outdated

Comment thread...nfiguration.Binder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T.generated.txt
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
@stevehartersteveharter
added
the 
source-generator
Indicates an issue with a source generator feature
label
Sep 13, 2023
@steveharter
steveharter
marked this pull request as ready for review
September 13, 2023 01:56
layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

@stevehartersteveharter
modified the milestone:
8.0.0Sep 14, 2023
layomia
layomia
approved these changes
Sep 14, 2023
View reviewed changes
@layomialayomia
left a comment


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!
@steveharter
steveharter
merged commit 650eec9
into
dotnet:mainSep 14, 2023
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 14, 2023 21:33
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Need to get in at least this before backport merge can be successful: #91717
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6191249484
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter backporting to release/8.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Make src gen for property setters consistent with reflection
.git/rebase-apply/patch:218: trailing whitespace.
Assert.Null(options.OtherCodeUri); warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/Types/SimpleTypeSpec.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/ParsableFromStringSpec.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make src gen for property setters consistent with reflection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter an error occurred while backporting to release/8.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@stevehartersteveharter
added this to the 8.0.0 milestone
Sep 14, 2023
@stevehartersteveharter
mentioned this pull request
Sep 15, 2023
 Merged

@steveharter
steveharter
restored the

GenPropertySet

branch
September 15, 2023 21:42
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 15, 2023 21:45
@ericstj
ericstj
commented
Sep 16, 2023

Copy link


Copy Markdown

Member
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 16, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6204180109
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 16, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 16, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghAwaiting requested review from tarekgh

1 more reviewer

@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-Configurationsource-generatorIndicates an issue with a source generator feature

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@steveharter@tarekgh@ericstj@layomia

, '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); }
})();
})();
Make src gen for property setters consistent with reflection by steveharter · Pull Request #91899 · dotnet/runtime · GitHub
Skip to content
Make src gen for property setters consistent with reflection - #91899
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Sep 14, 2023
Merged
Make src gen for property setters consistent with reflection#91899
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:GenPropertySet
Conversation
@steveharter
@stevehartersteveharter
commented
Sep 11, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Address #86333 (RC2 candidate as well)
  • Call the setter for value type properties (with the default value for the property type) if the value is not in the config, unless it is during a Bind (not "Get"). This requires a adding a bool arg to the BindCore() method.
  • Always check for a null value or missing section for properties and if null\missing (this was not done in all cases previously) then don't call the setter with the null. This addresses Config generator overwrites existing properties when provided with empty config section #91380.
Note that this PR matches known semantics regarding the above with the goal of maintaining compat with reflection; this is the case even for non-intuitive cases including:
  • Only value type properties get their setter called; not reference types or Nullable<T>.
  • Also, properties on objects that are nested objects or those added to a collection are also not defaulted through the setter (only properties on root objects are).
We could create an issue for these non-intuitive cases, but would need to consider the benefit:risk including breaking changes.
@stevehartersteveharter
self-assigned this
Sep 11, 2023
@ghost
ghost
commented
Sep 11, 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
null
Author:steveharter
Assignees:steveharter
Labels:
area-Extensions-Configuration
Milestone:-
@tarekgh
tarekgh
commented
Sep 12, 2023

Copy link


Copy Markdown

Member
@steveharter there are some failures that seem related to the change. Could you please have a look?
layomia
layomia
reviewed
Sep 12, 2023
View reviewed changes
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
Outdated

Comment thread...nfiguration.Binder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T.generated.txt
Comment thread...nder/tests/SourceGenerationTests/Baselines/OptionsBuilder/Bind_T_BinderOptions.generated.txt
@stevehartersteveharter
added
the 
source-generator
Indicates an issue with a source generator feature
label
Sep 13, 2023
@steveharter
steveharter
marked this pull request as ready for review
September 13, 2023 01:56
layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

layomia
layomia
reviewed
Sep 14, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
Outdated

@stevehartersteveharter
modified the milestone:
8.0.0Sep 14, 2023
layomia
layomia
approved these changes
Sep 14, 2023
View reviewed changes
@layomialayomia
left a comment


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!
@steveharter
steveharter
merged commit 650eec9
into
dotnet:mainSep 14, 2023
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 14, 2023 21:33
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Need to get in at least this before backport merge can be successful: #91717
@steveharter
steveharter
commented
Sep 14, 2023

Copy link


Copy Markdown

ContributorAuthor
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6191249484
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter backporting to release/8.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Make src gen for property setters consistent with reflection
.git/rebase-apply/patch:218: trailing whitespace.
Assert.Null(options.OtherCodeUri); warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/Types/SimpleTypeSpec.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/ParsableFromStringSpec.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make src gen for property setters consistent with reflection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 14, 2023

Copy link


Copy Markdown

Contributor
@steveharter an error occurred while backporting to release/8.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@stevehartersteveharter
added this to the 8.0.0 milestone
Sep 14, 2023
@stevehartersteveharter
mentioned this pull request
Sep 15, 2023
 Merged

@steveharter
steveharter
restored the

GenPropertySet

branch
September 15, 2023 21:42
@steveharter
steveharter
deleted the

GenPropertySet

branch
September 15, 2023 21:45
@ericstj
ericstj
commented
Sep 16, 2023

Copy link


Copy Markdown

Member
/backport to release/8.0
@github-actions
github-actionsBot
commented
Sep 16, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6204180109
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 16, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 16, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tarekghtarekghAwaiting requested review from tarekgh

1 more reviewer

@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-Configurationsource-generatorIndicates an issue with a source generator feature

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@steveharter@tarekgh@ericstj@layomia