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

Cleanup SyntaxNormalizerTests #65516

Merged
merged 10 commits into from
Dec 8, 2022
Merged

Conversation

DoctorKrolic
Copy link
Contributor

  • Converted multiline strings to raw form
  • Removed unnecessary NormalizeLineEndings calls
  • Made WorkItem attribute placement consistent: [Fact, WorkItem...]
  • Converted Theorys to Facts when inline data contains string with line breaks
  • Made usage of variables consistent (inlined them)
  • Made offered methods static

@DoctorKrolic DoctorKrolic requested a review from a team as a code owner November 19, 2022 13:31
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Nov 19, 2022
@DoctorKrolic
Copy link
Contributor Author

I've resolved all test fails on unix systems and ready for review now

@333fred
Copy link
Member

333fred commented Nov 28, 2022

@DoctorKrolic is this fixing any bugs? Or is this a style refactoring?

@DoctorKrolic
Copy link
Contributor Author

I would call it a style refactoring, but the main motivation here is readability. As per discussion in #65249 (comment) I was actually allowed to do this)

@RikkiGibson
Copy link
Contributor

  • Converted Theorys to Facts when inline data contains string with line breaks

I didn't understand the motivation for this part.

@DoctorKrolic
Copy link
Contributor Author

I didn't understand the motivation for this part.

As for me, long multiline strings as attribute parameters look really odd. Usually attributes contain some short easy-to-sequentially-read things. When they had single-line string it was kinda halfway right. When I converted them to raw strings, however, I decided, that it look better, when these values are somewhere in method body. + many tests already used this pattern with several test cases in single test method (even before my previous PR with initializers), so it also makes thing more consistent. I can totally revert this if you don't share this point of view.

@DoctorKrolic
Copy link
Contributor Author

@RikkiGibson ?

@RikkiGibson
Copy link
Contributor

@jcouv @CyrusNajmabadi for review

@DoctorKrolic
Copy link
Contributor Author

@jcouv @CyrusNajmabadi Still waiting for a review here...

@jcouv jcouv added the Test Test failures in roslyn-CI label Dec 8, 2022
@jcouv
Copy link
Member

jcouv commented Dec 8, 2022

We typically don't accept PRs that focus on code style. That said, given that this is a test-only PR (which therefore only needs one compiler review) and we have the required review, we'll go ahead and merge.
Thanks

@jcouv jcouv merged commit 7efeb7a into dotnet:main Dec 8, 2022
@ghost ghost added this to the Next milestone Dec 8, 2022
@DoctorKrolic DoctorKrolic deleted the cleanup-normalizer-tests branch December 8, 2022 19:09
@CyrusNajmabadi
Copy link
Member

Note: i don't personally feel this is code-style. These single-line tests are actually quite painful to work with, requiring lots of manual minging of \r\n in the comments, and basically often obfuscating what's actually going on (like what the test is testing against and what it is validating). A prime use case for raw-strings was exactly 'tests', with a bunch of enthusiasm from compiler side about being able to move these forward to this more maintainable form :)

@RikkiGibson
Copy link
Contributor

I'm very late on this, but thanks for the contribution @DoctorKrolic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. Test Test failures in roslyn-CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants