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

Add config of list indent size #21

Merged
merged 6 commits into from Dec 25, 2019

Conversation

@egs33
Copy link
Contributor

egs33 commented Nov 14, 2019

This PR is related to #8.

Add config of list indent size.
If this config is set to 1. Codes is formatted as below.

(foo
 bar
 baz)
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 14, 2019

Codecov Report

Merging #21 into master will increase coverage by 0.02%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   73.97%   73.99%   +0.02%     
==========================================
  Files          10       10              
  Lines        1145     1146       +1     
  Branches       27       27              
==========================================
+ Hits          847      848       +1     
  Misses        271      271              
  Partials       27       27
Impacted Files Coverage Δ
core/src/cljstyle/config.clj 98.09% <100%> (+0.01%) ⬆️
core/src/cljstyle/format/core.clj 97.05% <100%> (ø) ⬆️
core/src/cljstyle/format/indent.clj 94.07% <88.88%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b14b69...8a71ff2. Read the comment docs.

@egs33

This comment has been minimized.

Copy link
Contributor Author

egs33 commented Nov 21, 2019

clojure-style-guide suggested one space indent.

This PR enable its style.

@greglook greglook self-assigned this Dec 24, 2019
Copy link
Owner

greglook left a comment

This is great, thanks for tackling this! Based on the number of 👍 votes on #8 this will definitely be a popular feature. I have some comments on style and some consistency questions, but the logic generally seems sound. Once those small things are fixed it'll be good to go.

(Thanks for your patience on the review, I've been AFK travelling for a few weeks.)

README.md Outdated
@@ -217,6 +217,17 @@ disabled:
Require all files to end with a newline character. One will be added if it is
not present.

* `:list-indent-size`

This comment has been minimized.

Copy link
@greglook

greglook Dec 24, 2019

Owner

This is a significant enough setting that it probably belongs higher up, directly under :indentation?.

@@ -77,6 +78,7 @@
::single-import-break-width
::require-eof-newline?
::indents
::list-indent-size

This comment has been minimized.

Copy link
@greglook

greglook Dec 24, 2019

Owner

Consistently placing this key after :indentation? would be helpful too.

@@ -271,23 +271,23 @@

(defn- indent-line
"Apply indentation to the line beginning at this location."
[zloc indents]
(let [width (indent/indent-amount zloc indents)]
[zloc indents list-indent-size]

This comment has been minimized.

Copy link
@greglook

greglook Dec 24, 2019

Owner

I'll just note here that there's some inconsistency in how this argument is ordered throughout the rest of the changes here. Some places have it as a new final argument like this, others take it as a new second argument. Since this is the more basic of the two configuration arguments here, I think you should consistently place list-indent-size before indents and (later) the indenter rule arguments. That will also let you keep some partial forms intact later on.

[[rule-key opts]]
(apply some-fn (map (partial indenter-fn rule-key) opts)))
[list-indent-size [rule-key opts]]
(apply some-fn (map #(indenter-fn rule-key % list-indent-size) opts)))

This comment has been minimized.

Copy link
@greglook

greglook Dec 24, 2019

Owner

An example of the ordering inconsistency I mentioned above.

([form-string]
(reindent-string form-string config/default-indents))
([form-string indents]
(reindent-string form-string 2 config/default-indents))
([form-string list-indent-size]
(reindent-string form-string list-indent-size config/default-indents))
([form-string list-indent-size indents]
(reformat-string form-string {:indentation? true
:indents indents})))
:indents indents
:list-indent-size list-indent-size})))
Comment on lines 9 to 16

This comment has been minimized.

Copy link
@greglook

greglook Dec 24, 2019

Owner

Thanks for adding some new tests! The rest of the test code would have many fewer changes if you kept the two-argument version as-is accepting indents and used the three-argument version for the list-indent-size when you needed it. Seems like a lot of the existing tests just got a 2 inserted.

@egs33

This comment has been minimized.

Copy link
Contributor Author

egs33 commented Dec 25, 2019

Thanks for reviewing. I fixed them. Please re-review.

@egs33 egs33 requested a review from greglook Dec 25, 2019
Copy link
Owner

greglook left a comment

Looks good!

@greglook greglook merged commit 59e3311 into greglook:master Dec 25, 2019
5 checks passed
5 checks passed
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: style Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
codecov/patch 92.85% of diff hit (target 73.97%)
Details
codecov/project 73.99% (+0.02%) compared to 6b14b69
Details
@egs33 egs33 deleted the toyokumo:feature/list-indent-size-config branch Dec 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.