-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Ensure new files are formatted based on the right language version #57119
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
Conversation
ab291da to
49783af
Compare
|
Putting this up as draft while I wait for an answer from CPS. |
jasonmalinowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get an integration test added?
Also: target against the 17.0 branch?
src/VisualStudio/CSharp/Impl/LanguageService/CSharpEditorFactory.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs
Outdated
Show resolved
Hide resolved
The bug is currently targeting 17.1 P3, and has a score of 1. I'm assuming that when Dev17 GAs more people will find it and complain, so I like the simpler change for easier serviceability, but I'm happy not aggressively pushing it. |
|
Merged to try to get integration tests passing... and no, it wasn't my test that was failing 😛 |
| VisualStudio.SolutionExplorer.AddProject(project, WellKnownProjectTemplates.ConsoleApplication, LanguageNames.CSharp); | ||
|
|
||
| VisualStudio.ErrorList.ShowErrorList(); | ||
| VisualStudio.ErrorList.Verify.NoErrors(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also have a test for non-legacy project does work in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it can't hurt to have a non legacy test to ensure it compiles, but I can't get a failing test in non legacy, because I couldn't see a way to specify target framework when creating a project.
src/VisualStudio/IntegrationTest/IntegrationTests/CSharp/CSharpNewDocumentFormatting.cs
Show resolved
Hide resolved
jasonmalinowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small tweaks, but ![]()
|
Thanks for the review, I've pushed the updates but I'm still trying to understand this integration test failure which makes no sense (and today, is passing on my machine ¯\_(ツ)_/¯) |
Fixes AB#1411721
This fixes new project creation for legacy and CPS with respect to LangVersion issues.