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.

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

'Simplify interpolation' should not be suggested in VB for non-literal alignments #49717

Merged
merged 7 commits into from
Apr 19, 2021

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Dec 2, 2020

Fixes #49712

💭 Would like to revert 4365779 and bring back abstract helper classes. That way there wouldn't now be three pieces of knowledge duplicated between the analyzers and the fix providers: 1. what conditional syntax is 2. what parenthesized syntax is 3. whether the language supports non-literal alignment components

@jnm2 jnm2 changed the title VB only allows integral-typed literals as alignment components 'Simplify interpolation' should not be suggested in VB for non-literal alignments Dec 2, 2020
@jnm2 jnm2 marked this pull request as draft December 2, 2020 02:08
…edge to all langs instead of using inheritance"

This reverts commit 4365779.
@jnm2 jnm2 force-pushed the simplify_interpolation_vb_alignment branch from b60a850 to 5feb730 Compare December 2, 2020 04:50
@jnm2 jnm2 marked this pull request as ready for review December 2, 2020 04:58
@jnm2 jnm2 force-pushed the simplify_interpolation_vb_alignment branch from 9717e66 to 018c7aa Compare December 7, 2020 13:32
Base automatically changed from master to main March 3, 2021 23:53
@jnm2
Copy link
Contributor Author

jnm2 commented Apr 18, 2021

@sharwell Here's a case where fixing merge conflicts by merging instead of rebasing causes a situation where you really can't see what's happening at all. Do you know where the "merging is better" theory broke down in this case, so that I could have predicted it and rebased instead?

@sharwell
Copy link
Member

sharwell commented Apr 18, 2021

@jnm2 if someone had already reviewed the Files Changed for 018c7aa (and marked the files as reviewed in their local view), the re-review of the complete PR (all commits) would only uncheck the items which changed in the final merge commit. Otherwise, new reviewers are looking at a fairly clean change.

@jnm2
Copy link
Contributor Author

jnm2 commented Apr 18, 2021

@sharwell Oh, is it something they can benefit from and I can't? I was hoping to be able to have a view of what changed within the PR files in the diff of the merge commit, but it shows me everything, way too much to be useful.

@jnm2 jnm2 force-pushed the simplify_interpolation_vb_alignment branch from d8f7cd8 to 543371b Compare April 18, 2021 23:03
@sharwell
Copy link
Member

I think the diff is just too big to show the condensed diff. I imagine there is a git command line feature to show the equivalent locally.

@CyrusNajmabadi CyrusNajmabadi merged commit 497a472 into dotnet:main Apr 19, 2021
@ghost ghost added this to the Next milestone Apr 19, 2021
@jnm2 jnm2 deleted the simplify_interpolation_vb_alignment branch April 19, 2021 20:53
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VB.Net editor fails to use variables in for text width in String interpolation
5 participants