Fix indentation for symbols in reader conditionals #348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
camsaul
added a commit
to metabase/metabase
that referenced
this pull request
Aug 21, 2024
camsaul
added a commit
to metabase/metabase
that referenced
this pull request
Aug 21, 2024
* Cljfmt config part 2 * Part 3 WIP * Part 3 WIP * Part 3 WIP * Part 3 WIP * Part 3 WIP * Use fork with weavejester/cljfmt#348 and weavejester/cljfmt#350 * Backport updated config and linter fork from part 3 * Update formatting * Reformat * Fix bad indentation from #47064
weavejester
requested changes
Aug 22, 2024
cljfmt/src/cljfmt/core.cljc
Outdated
Comment on lines
252
to
258
| "Given a zipper pointing to a reader conditional form like | ||
|
|
||
| #?(:clj potemkin.core/defprotocol+ :cljs defprotocol) | ||
|
|
||
| Return the first symbol inside it e.g. | ||
|
|
||
| potemkin.core/defprotocol+" |
There was a problem hiding this comment.
Can you remove this docstring? Since this is a private function, we don't need to give it public documentation. Instead, perhaps improve the function name. Something like: first-symbol-in-reader-conditional.
cljfmt/src/cljfmt/core.cljc
Outdated
Comment on lines
261
to
264
| ;; find the first keyword e.g. `:clj` | ||
| (when-let [key-loc (z/find (-> zloc z/down z/right z/down) | ||
| z/right | ||
| #(n/keyword-node? (z/node %)))] |
There was a problem hiding this comment.
Maybe instead of the comment, we use a more descriptive function:
(when-key [key-zloc (-> zloc z/down z/right z/down find-next-keyword)]
cljfmt/src/cljfmt/core.cljc
Outdated
Comment on lines
265
to
266
| ;; now move to the next non-whitespace/non-comment/non-metadata node after | ||
| ;; the keyword node e.g. `potemkin.core/defprotocol+`. |
There was a problem hiding this comment.
I don't think we need this comment, the code seems clear enough, and if it isn't, we should split it out into more functions. Ideally comments should explain "why" rather than "what".
camsaul
force-pushed
the
fix-reader-conditional-symbols
branch
from
ce787be
to
606380b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Things like
were not indenting correctly despite having indentation specs defined for both
potemkin.core/defprotocol+anddefprotocol.This PR reworks
form-symbola little bit to return the first symbol it finds when it encounters#?forms -- in this case,potemkin.core/defprotocol+(meaning the form above will be formatted using that spec). Does not currently work with#?@reader conditionals, but I wasn't sure how that should realistically work anyway.