Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add feature to simplify code to use the new extended property patterns when available. #55704

Merged
merged 77 commits into from
Aug 20, 2021
Merged

Add feature to simplify code to use the new extended property patterns when available. #55704

merged 77 commits into from
Aug 20, 2021

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Aug 18, 2021 •
edited by jcouv
Loading

Looks like this:

image

Todo:

  • tests

Fixes #55545

Relates to test plan #52468

sharwell and others added 30 commits April 20, 2021 13:55
@sharwell
Implement analysis scope solution explorer command
2e9a907
@sharwell
Create IAnalysisScopeService and update solution crawler to use it
54ae35e 
Closes dotnet#52768
@sharwell
Enable nullable reference types for BackgroundCompiler.cs
79b43c1
@sharwell
Merge remote-tracking branch 'dotnet/main' into analysis-scope
cb11d81
@sharwell
Avoid unnecessary conversions to async code
6cad4b4
@sharwell
Merge remote-tracking branch 'dotnet/main' into analysis-scope
292b829
@sharwell
Store weak references in s_structuresTable
04b2f8a 
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1307181
@akhera99
indexer option first start
cc98811
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20210…
acb8afb 
…803.5

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk
 From Version 6.0.0-beta.21379.2 -> To Version 6.0.0-beta.21403.5
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20210…
39e42e7 
…806.6

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk
 From Version 6.0.0-beta.21379.2 -> To Version 6.0.0-beta.21406.6
@ryzngard
Fix interpolated string refactoring to work for Console.Write*
733429a
@ryzngard
Update to include more methods. Add more tests. Simplify some code
63fbea2
@JoeRobich
Reverted SDK version change
a963a0f
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20210…
1cedbb9 
…810.8

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk
 From Version 6.0.0-beta.21379.2 -> To Version 6.0.0-beta.21410.8
@akhera99
add option for array indexers
e00975e
@ryzngard
PR Feedback
0040fa4
@akhera99
changed string name
f12f938
@akhera99
made the option generic for any type of parameter
f8e8001
@sharwell
Merge remote-tracking branch 'dotnet/main' into analysis-scope
9f1ef81
@akhera99
added option in VB
3652792
@akhera99
removed balnk lines for some reason
e05c1db
@sharwell
Switch to an option persister for solution-specific analysis scope
f7ce1ee
@akhera99
Merge branch 'main' into add_indexer_option_inline_hints
d317c21
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20210…
a3eddbd 
…812.1

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk
 From Version 6.0.0-beta.21379.2 -> To Version 6.0.0-beta.21412.1
@sharwell
Document SolutionCrawlerOptions and revert unnecessary changes
d0506d7
@sharwell
Update test to account for manual option checks
0742bca 
Only analyzers that use SolutionCrawlerOptions internally need to
reanalyze when these options change. The tests are now updated to
reflect this change.
@akhera99
test
a78a7a6
@akhera99
Merge branch 'add_indexer_option_inline_hints' of https://github.com/…
dc6e64b 
…akhera99/roslyn into add_indexer_option_inline_hints
@akhera99
proper test
f096b50
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20210…
4174b3a 
…813.4

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk
 From Version 6.0.0-beta.21379.2 -> To Version 6.0.0-beta.21413.4
@sharwell
Merge pull request dotnet#55344 from sharwell/structures-table
96d6017 
🐇 Store weak references in s_structuresTable
@runfoapp runfoapp bot mentioned this pull request Aug 19, 2021
Open
333fred
333fred reviewed Aug 19, 2021
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Looks like there's a merge of commits that aren't yet in main, will hold off approving until those commits are merged and this PR shows a clean diff.

@CyrusNajmabadi CyrusNajmabadi merged commit 6254a02 into dotnet:release/dev17.1-preview1 Aug 20, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the simplePropertyPattern branch August 20, 2021 06:08
@333fred
Copy link
Member

333fred commented Aug 20, 2021

@CyrusNajmabadi it doesn't look like there was 2 compiler reviews. I was holding off on approving this until I could actually review a clean diff with the target branch, which is now impossible.

@CyrusNajmabadi
Copy link
Member Author

This shouldnt' have needed 2 compiler reviews afaict. All i did was merge main into this branch. Do you review those changes?

@333fred
Copy link
Member

333fred commented Aug 20, 2021

Any non-test changes that touch the compiler folder need 2 reviews. I cannot separate your changes from the merge, so no I was not able to review them.

@333fred
Copy link
Member

333fred commented Aug 20, 2021

Moreover, because this pr was merged bringing in a whole bunch of unrelated commits, it means we can't easily revert the changes if necessary. In the future, please do not merge a pr with unrelated commits in the merge because codeflow has not yet occurred.

@CyrusNajmabadi
Copy link
Member Author

Ok, i won't do in the future. This was done here though to unblock CI, which needed those changes to properly use sdk-preview7 and which had already been approved in #55681. It pulled in 'main'. But 'main' is already something we auto-approve for merge into later-release branches.

Given the contents were all approved already, i didn't know there needed to be additional approval. But in the future i can avoid this if it is problematic.

@333fred
Copy link
Member

333fred commented Aug 20, 2021

It pulled in 'main'. But 'main' is already something we auto-approve for merge into later-release branches.

Right, I understand this. However, main, was not yet merged into this branch. This means that your PR effectively combined your code and #55756 into a single merge commit, which makes it very difficult to separate your changes from the main changes. I'm not saying you shouldn't have merged main into your branch to allow CI to run: I'm saying you should have waited for #55756 to merge before merging this, so that this PR would be just your changes, and not your changes + everything in main since the last time it was merged into release/dev17.1-preview1.

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

@333fred 333fred 333fred left review comments

@jcouv jcouv jcouv approved these changes

@jmarolf jmarolf jmarolf approved these changes

Assignees

@jcouv jcouv

Labels
Area-IDE Feature Request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Offer to refactor into extended property pattern