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

Uniform code style #190

Merged
merged 11 commits into from
Nov 21, 2022
Merged

Uniform code style #190

merged 11 commits into from
Nov 21, 2022

Conversation

aurorarossi
Copy link
Member

It fixes some typos and spaces.

@codecov
Copy link

codecov bot commented Nov 20, 2022

Codecov Report

Merging #190 (00393ec) into master (4fce15b) will decrease coverage by 0.07%.
The diff coverage is 94.84%.

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
- Coverage   97.47%   97.40%   -0.08%     
==========================================
  Files         109      109              
  Lines        6349     6468     +119     
==========================================
+ Hits         6189     6300     +111     
- Misses        160      168       +8     

@gdalle
Copy link
Member

gdalle commented Nov 20, 2022

Hey @aurorarossi did you do this with JuliaFormatter or manually? Because it would be great to actually add a formatting check to the test suite with a consistent style (see #66). Gonna push this on your PR if that's OK

@aurorarossi
Copy link
Member Author

I have the following settings on vsc:

  • enabled Insert Final Newline
  • enabled Trim Final Newlines
  • enabled Trim Trailing Whitespace
  • disabled Ignore Trim Whitespace.

Yes of course! You can add the JuliaFormatter here

@gdalle
Copy link
Member

gdalle commented Nov 20, 2022

This is a great idea, thank you for starting it! I think the VSCode settings are less reliable than an explicit JuliaFormatter configuration (see here) with a well-known style like BlueStyle. Let's see how much of the code base it changes

@gdalle gdalle changed the title Housekeeping Uniform code style Nov 20, 2022
@simonschoelly
Copy link
Member

I think this PR also fixes so grammatical errors - now its a bit difficult to spot these parts with also these changes here - maybe we can make two PRs out of that?

src/Experimental/Experimental.jl Show resolved Hide resolved
src/Experimental/vf2.jl Show resolved Hide resolved
@@ -0,0 +1 @@
style = "blue"
Copy link
Member

Choose a reason for hiding this comment

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

BlueStyle seems like the most well-established specification, whereas SciML is more recent. However JuliaFormatter cannot do it all, so we must tell contributors to adhere to the style guidelines in general. Should we add a badge to the README too?

Copy link
Member

@simonschoelly simonschoelly Nov 20, 2022

Choose a reason for hiding this comment

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

Any reason to prefer BlueStyle? (I am not yet familiar with these styles). We could also specify our own rules right?

Copy link
Member

Choose a reason for hiding this comment

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

In any cases, most of the changes here seem to be sensible.

Copy link
Member

Choose a reason for hiding this comment

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

I just use it because it's already well documented, so we can point contributors to the exhaustive specification by Invenia. We could make up our own rules but that would require explaining them to everyone in our docs.

Also, it's easier to contribute when you're already familiar with the style guide, which wouldn't be the case for Graphs-specific rules.

Copy link
Member

Choose a reason for hiding this comment

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

My hot take is that the precise choice of style does not matter, as long as it's clear and easy to apply, and BlueStyle satisfies both criteria

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to use some common style, as then people don't need to reconfigure their editor differently for every project.

Copy link
Member

Choose a reason for hiding this comment

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

What about the default format by JuliaFormatter? Would that also be an option?

Copy link
Member

Choose a reason for hiding this comment

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

The default format by JuliaFormatter is much less well-specified than BlueStyle. One of the perks of BlueStyle is that it also gives coherent writing advice to the contributors that cannot be enfored by a simple linter

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to use some common style, as then people don't need to reconfigure their editor differently for every project.

Another perk of JuliaFormatter is that the formatting in the Julia extension for VSCode is based on it. And it takes into account the .JuliaFormatter.toml file at the root of the package to format each file according to the local specification (in our case style = "blue")

src/Graphs.jl Show resolved Hide resolved
@gdalle
Copy link
Member

gdalle commented Nov 20, 2022

I think this PR also fixes so grammatical errors - now its a bit difficult to spot these parts with also these changes here - maybe we can make two PRs out of that?

You're right, it's hard to spot but you can see them during code review if you filter by commit (to keep only the ones by @aurorarossi) and specify that you don't want to see whitespace modifications. I reviewed all of these changes and they look okay to me.

I just wanted to do code formatting in the same PR because if we merged @aurorarossi's PR alone it would have whitespace changes that are incoherent with the BlueStyle

@gdalle gdalle mentioned this pull request Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants