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

Results from applying fix in v.0.5.0 to Wolframite #102

Closed
holyjak opened this issue Oct 5, 2024 · 11 comments
Closed

Results from applying fix in v.0.5.0 to Wolframite #102

holyjak opened this issue Oct 5, 2024 · 11 comments

Comments

@holyjak
Copy link

holyjak commented Oct 5, 2024

Hello! I very much applaud this work! As asked, I have tried to apply fix to our library Wolframite, and here are the results: scicloj/wolframite#134

Some very nice changes but also some weird / incorrect / questionable:

  1. :require vectors are aligned below the r - as Cursive user I am used to that, but Emacs tends to align below :. I agree with the choice :-) but want to highlight that this may be a source of disagreements
  2. Here I had on purpose a line break in a long require for readability (and b/c we may want to have lines not too long). Not here but at work we enforce particular line length, so if the formatter forces the whole require on a single line, we've a problem.
  3. Here a , is moved from end of line to the start of the next line; why?!
  4. Here an empty line between defns is removed, which seems wrong.
  5. I like to separate pairs of cond-action in cond by an empty line, to make it clear what these pair are, but the formatter removes them
@oakmac
Copy link
Owner

oakmac commented Oct 5, 2024

Thank you for the report and the kinds words 🤓

I will look into these.

@oakmac
Copy link
Owner

oakmac commented Oct 5, 2024

  1. :require vectors are aligned below the r - as Cursive user I am used to that, but Emacs tends to align below :. I agree with the choice :-) but want to highlight that this may be a source of disagreements

FYI - this is tracked at Issue #87

@oakmac
Copy link
Owner

oakmac commented Oct 5, 2024

  1. Here a , is moved from end of line to the start of the next line; why?!

This is a bug. Tracked at Issue #104

  1. Here an empty line between defns is removed, which seems wrong.
  1. I like to separate pairs of cond-action in cond by an empty line, to make it clear what these pair are, but the formatter removes them

I suspect these are the same bug. Tracked at Issue #103

@oakmac
Copy link
Owner

oakmac commented Oct 6, 2024

  1. Here I had on purpose a line break in a long require for readability (and b/c we may want to have lines not too long). Not here but at work we enforce particular line length, so if the formatter forces the whole require on a single line, we've a problem.

This is a tricky case. I think the best answer is "put it all on one line" (ie: the current behavior). Standard Clojure Style prints ns forms "from scratch" so I am not sure there will ever be a "one size fits all" algorithm for how to split up a long :require line.

I think the simplest behavior is:

  1. For each :require line, put it all on one line.
  2. If a line is too long and this effects readability, consider refactoring to make it shorter.

I realize that it seems strange that a code formatter would recommend "refactor your code" as a solution, but I think for long :require lines this advice is likely always advantageous. It could be that this constraint of the formatter nudges people towards cleaner and smaller :require forms; this is a good thing IMO.

In clojure namespace aliases we have the advice of "Use :refer sparingly".

From how to ns there is a recommendation of "Do not :refer :all."

@NoahTheDuke
Copy link

If a line is too long and this effects readability, consider refactoring to make it shorter.

cljfmt will add line breaks at 80 chars in refer vectors. It doesn't necessarily look great but it works better than lines that are 200 characters long lol.

@holyjak
Copy link
Author

holyjak commented Oct 8, 2024

Regarding the long require and

  1. For each :require line, put it all on one line.
  2. If a line is too long and this effects readability, consider refactoring to make it shorter.

and the general advice on refer an use - in general, I agree with the rules. But as is always the case with rules, they are too limited to capture the richness of the world. In this particular case of this special library, what we do makes a very good sense. I wouldn't want to compromise user UX to make a formatter happy.

What Noah suggests makes sense to me - if the line is too long, insert a break around e.g. 80. May not be beautiful but it works and is consistent....

@NoahTheDuke
Copy link

NoahTheDuke commented Oct 8, 2024

Here's an example file that uses the "newline at N characters" rule: https://github.com/mtgred/netrunner/blob/master/src/clj/game/cards/agendas.clj

Rewriting this to use aliases would make the require block prettier at the expense of the rest of the file.

@imrekoszo
Copy link
Contributor

imrekoszo commented Oct 8, 2024

One could argue that if a line length limit applies in some places it should perhaps apply everywhere for consistency's sake.

@oakmac
Copy link
Owner

oakmac commented Oct 16, 2024

But as is always the case with rules, they are too limited to capture the richness of the world. In this particular case of this special library, what we do makes a very good sense. I wouldn't want to compromise user UX to make a formatter happy.

This is well said and I agree 100%. There will always be special cases and good reasons why someone might want to bypass the formatter for some particular chunk of code or an entire file.

v0.8.0 adds support for #_:standard-clj/ignore to ignore the next form (including ns forms), and #:standard-clj/ignore-file to ignore an entire file.

@oakmac
Copy link
Owner

oakmac commented Oct 19, 2024

FYI @holyjak - v0.10.0 contains several fixes related to the issues you reported here

@oakmac
Copy link
Owner

oakmac commented Oct 21, 2024

I just ran standard-clj fix on the Wolframite codebase, ignoring src/wolframite/wolfram.clj and src/wolframite/wolfram_extended.clj, and produced this diff.

This is subjective of course, but these changes look good to me! I think in general they are an improvement to the codebase.

Thank you again @holyjak for the initial report 🙏 It was helpful to find some important bugs with the formatter.

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

No branches or pull requests

4 participants