Skip to content

Run cljfmt fix src test build.clj#26

Merged
cap10morgan merged 1 commit intomainfrom
12-15-Run_cljfmt_fix_src_test_build.clj
Jan 4, 2024
Merged

Run cljfmt fix src test build.clj#26
cap10morgan merged 1 commit intomainfrom
12-15-Run_cljfmt_fix_src_test_build.clj

Conversation

@cap10morgan
Copy link
Contributor

No description provided.

@cap10morgan cap10morgan requested a review from a team December 15, 2023 19:49
@cap10morgan cap10morgan mentioned this pull request Dec 15, 2023
Copy link
Contributor Author

cap10morgan commented Dec 15, 2023

Copy link
Contributor

@dpetran dpetran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍾

@@ -11,10 +11,10 @@
(log/debug "Starting raft consensus with config:" config)
(let [handler (consensus-handler/create-handler consensus-handler/default-routes)]
(consensus/start handler (assoc config :join? false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure if cljfmt can handle this, but i think multi-line assoc calls should have their first key-value pair start on a new line so that all the k-v pairs line up for easy scanning. here, the :join? false is on a different column from all the other pairs, so someone doing a quick scan to see which attributes are set to one could easily miss it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think it can enforce that.

(io-file/connection-storage-read
log-directory
encryption-key))
log-directory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think i prefer 2-space indentation for function calls when all the arguments are on a new line. i think it better expresses the semantic difference between the function and the arguments it takes. but i'm also ok with leaving it this way if other folks disagree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually just cljfmt treating lists like other multiline compound data literals. For example:

[:foo
 :bar
 :baz]

I'm not sure if you can tell it to make an exception for lists or if you'd have to make all such data literals indent w/ two spaces on subsequent lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I was wrong and this is specifically configurable in cljfmt: https://github.com/weavejester/cljfmt#formatting-options (see :function-arguments-indentation section). I'm happy to go w/ whatever the team consensus is here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like 2-space indentation here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you all have a preference between the :cursive and :zprint options for that param?

Copy link
Contributor

@zonotope zonotope Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh. seeing it so strongly rejected in the style guide (from the link in the description of the "community" default option) is making me rethink my stance on this. perhaps we shouldn't go against the default.

(invalid-address-shutdown
(str "Error in multi-address type in: " multi-addr
" Fluree only supports the ipv4, ipv6, dns4 or dns6 at the moment.")))
(str "Error in multi-address type in: " multi-addr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is an even better example of why 2 space indentation for function calls is more legible.

@@ -1,30 +1,30 @@
(ns fluree.server.components.http
(:require
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stuart Sierra notwithstanding, I prefer the first ns require to be on the same line as the :require keyword. I think the extra indentation that gives helps break up the require blocks vs imports, uses, and refer-clojures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree, but I think this is another thing that cljfmt cannot (yet?) enforce.

Copy link
Contributor Author

cap10morgan commented Jan 2, 2024

Merge activity

  • Jan 2, 2:26 PM: @cap10morgan started a stack merge that includes this pull request via Graphite.
  • Jan 2, 2:26 PM: Graphite couldn't merge this PR because it had conflicts with the trunk branch.
  • Jan 4, 2:20 PM: @cap10morgan started a stack merge that includes this pull request via Graphite.
  • Jan 4, 2:20 PM: @cap10morgan merged this pull request with Graphite.

Base automatically changed from 12-15-Add_cljfmt_config to main January 2, 2024 19:26
@cap10morgan cap10morgan force-pushed the 12-15-Run_cljfmt_fix_src_test_build.clj branch from 51e1db8 to ade62aa Compare January 2, 2024 20:02
@cap10morgan cap10morgan requested a review from a team January 2, 2024 20:02
@cap10morgan cap10morgan force-pushed the 12-15-Run_cljfmt_fix_src_test_build.clj branch from ade62aa to db9c944 Compare January 4, 2024 19:14
@cap10morgan cap10morgan merged commit 0f1682c into main Jan 4, 2024
@cap10morgan cap10morgan deleted the 12-15-Run_cljfmt_fix_src_test_build.clj branch January 4, 2024 19:20
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

Successfully merging this pull request may close these issues.

3 participants