search

oakmac / standard-clojure-style-js

Standard Clojure Style in JavaScript
ISC License
84 stars 1 forks source link

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

Closed holyjak closed 1 month ago

holyjak commented 1 month ago

Hello! I very much applaud this work! As asked, I have tried to apply fix to our library Wolframite, and here are the results: https://github.com/scicloj/wolframite/pull/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 commented 1 month ago

Thank you for the report and the kinds words :nerd_face:

I will look into these.

oakmac commented 1 month ago
  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 commented 1 month ago
  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.

  2. 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 commented 1 month ago
  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 commented 1 month ago

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 commented 1 month ago

Regarding the long require and

  1. For each :require line, put it all on one line.
    1. 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 commented 1 month ago

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 commented 1 month ago

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

oakmac commented 1 month ago

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 commented 1 month ago

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

oakmac commented 1 month ago

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 :pray: It was helpful to find some important bugs with the formatter.