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

Figure out consistent code style #1765

Open
ToucheSir opened this issue Nov 12, 2021 · 9 comments
Open

Figure out consistent code style #1765

ToucheSir opened this issue Nov 12, 2021 · 9 comments

Comments

@ToucheSir
Copy link
Member

I've seen a few of files now in FluxML repos where the diffs ping-pong back and forth between different styles. Most of the discrepancy seems to be around function and type signature whitespace. Trailing whitespace too, my editor config is set to trim them and a surprising number of core files seem to have them.

ChainRulesCore recently added JuliaFormatter to their CI and it seems to be working well, but even just a couple of sentences in CONTRIBUTING.md would go a long way.

@DhairyaLGandhi
Copy link
Member

We can add a line to contributing.md, but we follow two spaces and no trailing whitespace. Additionally we use linux file endings. We use whitespace around types and around default arguments too (kwarg = value).

@ToucheSir
Copy link
Member Author

Well that's just the thing, the current codebase is anything but self-consistent! Just going through the top level of src/, https://github.com/FluxML/Flux.jl/blob/master/src/deprecations.jl has no spaces while https://github.com/FluxML/Flux.jl/blob/master/src/functor.jl does. A big advantage of auto-formatting is that it's hard to argue with a machine 😄, but regardless of whether we add one there should be some sort of rough consensus on this style question.

@DhairyaLGandhi
Copy link
Member

Yeah deprecations.jl is not following the style and should be fixed.

@Chandu-4444
Copy link

I'd like to help with this issue. Can you tell me what can I do?

@ToucheSir
Copy link
Member Author

Well, the first step is to get most contributors to agree on a convention :P. You could try doing something like the ChainRulesCore PR linked above and see what people say.

@darsnack
Copy link
Member

I like four spaces because it matches docstrings/markdown, but I'm happy with Dhairya's suggestion.

@theabhirath
Copy link
Member

Now that there is a consistent code style for Metalhead, could this be extended to the Flux repo as well?

@ToucheSir
Copy link
Member Author

That would be great. Something like https://github.com/JuliaDiff/ChainRulesCore.jl/blob/main/.github/workflows/format.yml would be nice to have too. We can trial this on a smaller FluxML repo if Flux turns out to hit too many formatting edge cases.

@theabhirath
Copy link
Member

I was about to set that up for Metalhead but decided to hold off until domluna/JuliaFormatter.jl#604 is resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants