TidierOrg / Tidier.jl

Meta-package for data analysis in Julia, modeled after the R tidyverse.
MIT License
515 stars 14 forks source link

Format code according to blue style and add checking workflow #71

Closed pitmonticone closed 1 year ago

pitmonticone commented 1 year ago

Changes

kdpsingh commented 1 year ago

@pitmonticone, I really appreciate the time and effort you've put into this commit. I have mixed feelings about strict adherence to the Blue style guide. I understand that adhering to it would bring this package in line with others in the ecosystem, particularly as more folks start to contribute. However, there are things I'm not a huge fan of, which I'll mention here:

So after a fair bit of thought, I think I'm going to hold off on adopting a formal style guide and enforcement mechanism. What I will do instead for now is put together some general rules of thumb for contributors.

I'm going to close this PR for now. That's not in any way a rebuke of the effort you've put in but just my opinion that I think it's too early for this package to do it along with my view that this package may be a slight outlier in style given its intended user base.

kdpsingh commented 1 year ago

I'll also just mention that the R tidyverse style guide uses 2 spaces of indent for likely similar reasons (see Section 4.2: https://style.tidyverse.org/pipes.html).

While Tidier.jl is obviously a Julia package, I think that I may (intentionally) adopt a more mixed style that is familiar to R users for Tidier as some of the contributions for this package will likely come from R users.

pitmonticone commented 1 year ago

Hi @kdpsingh, thank you for your rapid review and helpful comments.

[...] using 4 spaces really crops the code and makes it harder to screenshot, particularly on mobile devices. [...] I like the other things you've done to make the code spread out over multiple lines when it runs too long (which I will adopt), but I will stick with 2 spaces indent for now given the heavy use of piping.

This indentation issue can be immediately solved by adding indent = 2 in the .JuliaFormatter.toml configuration file as I've just done in the new commit in the code-formatting branch.

I really don't like DataFrame(; a= 1:3). I know why the semi-colon is there, but it introduces an unnecessary thing that R users need to grapple with as they attempt to use Julia.

This semicolon issue can be immediately solved by adding separate_kwargs_with_semicolon = false in the .JuliaFormatter.toml configuration file as I've just done in the new commit in the code-formatting branch.

As some folks working on this package are new to Julia, I don't want to introduce any unnecessary barriers to folks submitting PRs. I am still involved heavily enough that I edit commits for internal style consistency before merging. If the project grows to a point where a larger team is managing it, then I think we would want to adopt a more formal style guide and process at that point.

I see your point, I believe it makes perfect sense.

I'm going to close this PR for now. That's not in any way a rebuke of the effort you've put in but just my opinion that I think it's too early for this package to do it along with my view that this package may be a slight outlier in style given its intended user base.

No problem at all for me. Glad to help even more when I find the time in the near future.

kdpsingh commented 1 year ago

That's actually super helpful to know. I really wasn't sure how flexible the styling was. Good to see that those options are possible.

I'm still leaning towards delaying implementation of a style guide but let me reopen this PR just to play a bit more with the options.

For now, I agree with leaving off the Blue badge from the other commit (as you've done). If I adopt this I'll consider adding it back in. I may re-close in the future but will re-open to explore it further. Thanks again.

pitmonticone commented 1 year ago

Thank you for the re-opening @kdpsingh, you might want to consult the JuliaFormatter.jl documentation to explore all the implemented style guides and, more broadly, all the tunable formatting parameters.

kdpsingh commented 1 year ago

Thank you @pitmonticone, I will check it out!

kdpsingh commented 1 year ago

@pitmonticone, I love how configurable JuliaFormatter.jl is. Thank you for introducing me to it. I think we will almost certainly adopt this tool -- it's just a question of when.

Given that this project is still growing, I really want to focus initial efforts on functionality and readability rather than style. Readability and style are certainly connected, but I think we should revisit style once this project matures a bit more.

I'm going to close this PR for now but will keep your GitHub actions script and JuliaFormatter configuration handy. We will revisit this issue in the future.

Thank you again!

Karandeep

pitmonticone commented 1 year ago

Glad to know that @kdpsingh and thank you for starting such an ambitious and needed project.