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

ci: add a workflow to check code format. #709

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

AsakusaRinne
Copy link
Collaborator

As the size of our project is increasing, it seems necessary to add an .editorconfig file to specify how the code should be formatted. Besides, a workflow is included in this PR to check the code format.

To ensure passing the check, the only thing needed to do is running the following command before the PR.

dotnet format

@@ -10,7 +10,7 @@ internal static class DictionaryExtensions
return GetValueOrDefaultImpl(dictionary, key, defaultValue);
}
#elif !NET6_0_OR_GREATER && !NETSTANDARD2_1_OR_GREATER
#error Target framework not supported!
#error Target framework not supported!
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the old syntax - for the same reason you indent any other if statement!

@martindevans
Copy link
Member

Personally I'm not a big fan of auto formatters, I'm of the opinion they're a good way to make sure you're formatting isn't bad but also a good way to ensure it is uniformly mediocre! Is it possible to add this in a "softer" way - e.g. generate warnings that are surfaced as part of CI, but aren't a hard failure?

As for the changeset itself, it's pretty huge and difficult to review! Can this be broken up into smaller PRs? e.g. apply it locally, and submit small groups of files with related changes. As an example, one thing I have avoided doing for a long time is replacing namespace X { } with namespace X; because that generates an enormous amount of noise in the change (touching every single line in every file).

@AsakusaRinne
Copy link
Collaborator Author

TBH I'm not strongly pushing this changeset. It's okay for me to keep what the code was.

add this in a "softer" way - e.g. generate warnings that are surfaced as part of CI, but aren't a hard failure?

I prefer not using warning because most people seldom care the warning! I think we could disable the workflow but leave .editorconfig there to let user know what is the format configuration of our code (VS and Rider both support formatting on saving depending on .editorconfig). The .editorconfig in this PR was generated from format auto-detection of jetbrains Rider.

@martindevans
Copy link
Member

I think we could disable the workflow but leave .editorconfig there to let user know what is the format configuration of our code

Yeah I think this is a better way to go for now. That way we can submit smaller PRs over time to bring the overall codebase better in line with the config, and also adjust the config when it's wrong.

@AsakusaRinne AsakusaRinne merged commit 7b03e73 into SciSharp:master Apr 30, 2024
4 checks passed
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.

2 participants