Skip to content

Refact select fix foreach & PullMemberUp #36729

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
Merged
petrroll merged 22 commits into from
Jun 28, 2019
Merged

Refact select fix foreach & PullMemberUp #36729

petrroll merged 22 commits into from
Jun 28, 2019

Conversation

@petrroll
Copy link
Contributor

@petrroll petrroll commented Jun 25, 2019 •
edited
Loading

Further work on #35180

Based on #36592 that can't be merged due to conflicts described in #36697.
Only the last commit is new.

@petrroll petrroll requested review from a team as code owners June 25, 2019 01:33
@petrroll petrroll changed the base branch from master to release/dev16.3-preview1 June 25, 2019 01:34
petrroll
petrroll commented Jun 25, 2019
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Jun 25, 2019
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Jun 25, 2019
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Jun 25, 2019
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Jun 25, 2019
View reviewed changes
@petrroll
Copy link
Contributor Author

petrroll commented Jun 25, 2019 •
edited
Loading

I see how the helpers aren't syntax facts but they're sort of something similar so I could imagine them being in "similar" thing. They can't be in RefactoringHelpers directly because that one doesn't reference language spacific packages and the helpers need language specific SyntaxNode types. And I doubt we want to create new dependency just for that.

PS: I know we wanted to do one refactoring at a time but both of these changed the surface of the helper so I didn't see the benefit of doing one review and then reworking what has been reviewed.

@petrroll petrroll force-pushed the refact-select-fix-foreach branch from 68f192d to 9f17237 Compare June 25, 2019 20:50
@petrroll petrroll changed the title [WIP] Refact select fix foreach [WIP] Refact select fix foreach & PullMemberUp Jun 25, 2019
@petrroll
Copy link
Contributor Author

petrroll commented Jun 25, 2019

Ok, so I've moved the PullMemberUp to the new helper which required some changes but I feel like now it's way more robust while not being all that much more complex.

I'll look into moving the stuff to to a ILanguageService now.

@petrroll
Copy link
Contributor Author

petrroll commented Jun 25, 2019

Moved to ILanguageService, please review

@petrroll petrroll changed the title [WIP] Refact select fix foreach & PullMemberUp Refact select fix foreach & PullMemberUp Jun 25, 2019
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jun 27, 2019

Done with review.

Notes:

  1. any time i say nit i'm effectively saying: consider, but this is totally up to you if you prefer things this way.
  2. conceptual comments can be very helpful for the long term maintenance of hte code. They help people (including your older self 2 years from now), understand what you were trying to accomplish, which can help understand the right sorts of fixes to apply in the future. Often when bug fixing the "right location" isn't clear to people, and they might fix a symptom rather than a root cause. Having as much understanding as possible placed into the code helps address that.

Cheers!

@jinujoseph jinujoseph changed the base branch from release/dev16.3-preview1 to master June 28, 2019 00:29
@jinujoseph jinujoseph added this to the 16.3.P2 milestone Jun 28, 2019
@petrroll petrroll force-pushed the refact-select-fix-foreach branch from 13b9423 to 59a78d7 Compare June 28, 2019 17:45
@petrroll
Copy link
Contributor Author

petrroll commented Jun 28, 2019

Added docs, ready to merge.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jun 28, 2019

petrroll force-pushed the petrroll:refact-select-fix-foreach branch from 13b9423 to 59a78d7 37 minutes ago

FWIW, this completely breaks my review flow. Can you avoid force-pushing until you merge?

@petrroll
Copy link
Contributor Author

petrroll commented Jun 28, 2019

Yep sure, it's a bad habit anyway.

CyrusNajmabadi
CyrusNajmabadi approved these changes Jun 28, 2019
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Just nits.

Approve
Submit feedback approving these changes.

@petrroll petrroll merged commit 5374589 into dotnet:master Jun 28, 2019
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jun 28, 2019

Woohoo! Awesome job :)

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

Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

+1 more reviewer

@mavasani mavasani mavasani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

16.3.P2

Development

Successfully merging this pull request may close these issues.

4 participants

@petrroll @CyrusNajmabadi @mavasani @jinujoseph