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

Nuget lint #8930

Merged
merged 13 commits into from
Feb 7, 2024
Merged

Nuget lint #8930

merged 13 commits into from
Feb 7, 2024

Conversation

trejjam
Copy link
Contributor

@trejjam trejjam commented Jan 29, 2024

Dotnet has task format, using that, we can format and enforce some codestyle

@trejjam trejjam requested a review from a team as a code owner January 29, 2024 20:06
@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Jan 29, 2024
@trejjam trejjam mentioned this pull request Jan 29, 2024
Copy link
Collaborator

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR and helping us stay clean with our codestyle.

Unfortunately the Roslyn formatter does not give us an options to control the formatting of all code structures and there are some intentional formatting decisions that are not captured in the .editorconfig. This is typically not an issue as dotnet format will not change the formatting of this code. However, there are some changes that the Rider/Resharper formatter made that go against our intention as the formatting configuration is not complete.

Would you be willing to update the Rider configuration to ignore object and list initializers as well as line length? Otherwise, we will need to work out the appropriate configuration.

@trejjam
Copy link
Contributor Author

trejjam commented Jan 30, 2024

Thank you for the review I will process it shortly

Copy link
Collaborator

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Thanks for updating this.

After thinking about this more the quickest way to get these changes in is to ask you to revert the Resharper formatting changes and remove the Resharper config. Since the maintainers of this C# do not use this formatter, it would take a bit of work to get the configuration right so that it broke up long lines in a familiar and expected way.

If this is something you feel passionately about, I would be happy to work with you in a separate issue/pr to figure out what the correct Resharper configuration would be.

nuget/helpers/lib/NuGetUpdater/.editorconfig Outdated Show resolved Hide resolved
nuget/helpers/lib/NuGetUpdater/.editorconfig Outdated Show resolved Hide resolved
@trejjam
Copy link
Contributor Author

trejjam commented Jan 31, 2024

The added Resharper config is there to conform with the existing code. If I remove it, anyone who makes an update with VS + Resharper / Rider will create style changes, which are again used code style.

It only makes the life of contributors easier, the default tooling does not need to understand it.

I aim to have such a configuration for dotnet format that it conforms with the current code.

Of course, we can spend hours writing formatters that will ensure strictness. That will undoubtedly be awesome, but I do not see a real value.

If you insist that all configurations must be used by tooling, there are some possibilities. The easier one is to use Resharper CLI, which comes as a Nuget package. It's stricter than the dotnet format, but also slower and requires more configuration.

Another option is to use, e.g., csharpier (I am not a fan of it, but it's an option).


About current changes, the only one I am aware of is an indention of raw string literals, which was written in many different ways in this codebase. I only unified it into one style.

@JoeRobich
Copy link
Collaborator

I aim to have such a configuration for dotnet format that it conforms with the current code.

I believe this is the crux of the issue. The configuration is for dotnet-format but once you run Rider's formatter on the code, it starts making changes that dotnet-format wouldn't. If we can get the Rider's formatter to not break long lines, then I believe we will be in much better shape.

@trejjam trejjam force-pushed the feature/nuget-lint branch 2 times, most recently from bc0a308 to 88a1309 Compare February 1, 2024 13:22
@trejjam trejjam force-pushed the feature/nuget-lint branch 2 times, most recently from 8f867f5 to c098020 Compare February 5, 2024 13:20
@trejjam
Copy link
Contributor Author

trejjam commented Feb 5, 2024

I believe I addressed all comments and that the PR is ready.

Copy link
Collaborator

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Thanks @trejjam! I added suggestions for the remaining places we had closing parens on the following line. I also want to add back the removed setting as commented out because we will certainly want to reenable it once the Rider formatter bug is fixed. I know there has been a lot of back and forth on this PR. Thanks for sticking with it.

Copy link
Collaborator

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Thanks @trejjam! The test to validate our formatting with dotnet-format is a great addition. Wish I had thought of it. Also, the config changes to interact better with the Rider formatter are good for the community.

@trejjam
Copy link
Contributor Author

trejjam commented Feb 7, 2024

Thank you for all the reviews, I like to push things forward :)

@abdulapopoola abdulapopoola merged commit a9e814a into dependabot:main Feb 7, 2024
53 checks passed
@abdulapopoola
Copy link
Member

Thanks a lot @trejjam for all the help!

@trejjam trejjam deleted the feature/nuget-lint branch February 8, 2024 06:09
@CharliePoole
Copy link

I'm having this issue as well. Looking for the best workaround until the fix is released.

I could remove the grouping from my yml files but then I'd have to put them back later. What I'm thinking is to simply merge the change and then let dependabot re-discover the updates in a subsequent pass. Will that work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants