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

Port CSharpUseLocalFunction analyzer/tests to shared layer #43178

Merged
merged 2 commits into from
Apr 13, 2020

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented Apr 8, 2020

First commit: file moves
Second commit: xlf and resx changes

Porting the code fix is blocked on #43056

@mavasani mavasani added this to the 16.7 milestone Apr 8, 2020
@mavasani mavasani requested review from CyrusNajmabadi and a team April 8, 2020 03:02
@CyrusNajmabadi
Copy link
Member

Porting the code fix is blocked on #43056

I would prefer to not move then...

If someone turns this on, but can't fix, that seems unfortunate :(

@mavasani
Copy link
Contributor Author

mavasani commented Apr 8, 2020

@CyrusNajmabadi the code fix is still available from the fixer that ships with IDE. It is just not available in NuGet package, so will not be available if executing a fix outside VS (for example some command line tool or VS code or such).

@mavasani
Copy link
Contributor Author

Ping @CyrusNajmabadi - let me know if you disagree with the justification.

@CyrusNajmabadi
Copy link
Member

Ping @CyrusNajmabadi - let me know if you disagree with the justification.

I don't like it. I think we should not do this if we can't move the whole feature over (analyzer and diagnostic). I worry that we'll just end up with crap left behind that we never get to. But not moving this at all, it highly motivates solving this in an appropriate manner.

@mavasani
Copy link
Contributor Author

I don't like it. I think we should not do this if we can't move the whole feature over (analyzer and diagnostic). I worry that we'll just end up with crap left behind that we never get to. But not moving this at all, it highly motivates solving this in an appropriate manner.

From an end-user perspective, this will make no difference at all. They just need the analyzer on CI, and code fix in IDE (regardless of whether it is invoked from a NuGet package or from an implementation in IDE). My point is, do we think it is worth blocking a user feature (CI enforcement) on just the fact that we'd like to cleanup and port the fixer to NuGet package?

@mavasani mavasani added the Need Design Review The end user experience design needs to be reviewed and approved. label Apr 13, 2020
@mavasani
Copy link
Contributor Author

@CyrusNajmabadi Can you review based on the discussions in meeting today? This PR is just moving an existing analyzer + tests to shared layer. Code fix port is blocked on CodeGeneration service becoming a public API.

@mavasani
Copy link
Contributor Author

Thanks!

@mavasani mavasani merged commit d8e8df8 into dotnet:master Apr 13, 2020
@ghost ghost modified the milestones: 16.7, Next Apr 13, 2020
@mavasani mavasani deleted the PortNewAnalyzer branch April 13, 2020 22:01
@mavasani mavasani removed the Need Design Review The end user experience design needs to be reviewed and approved. label Apr 13, 2020
@sharwell sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants