Introduce `:overrides` option #152

vemv · 2020-04-01T23:34:49Z

Brief

Implements #134 (comment)

Starting from this PR, :overrides becomes the primary API for customizing things: as a consumer, one should:

Use one of: git-status-formatter, branch-formatter, project-formatter
Pass :overrides to those if wanting to tweak something.

...and should not:

Use core/format! directly
Use the "factory" stuff introduced in this PR

TODO

Address XXXs
At this point, git-status-formatter, branch-formatter, project-formatter all deserve some test coverage.
Explore how much can we DRY git-status-formatter, branch-formatter, project-formatter
- A nice 'overrides' implementation would allow absolute DRY.
- Might be less prioritary for now
Create impl sub-nses for git-status-formatter, branch-formatter, project-formatter
- The intended API is format-and-lint! / lint!
- Things like speced/defn default-formatters are certainly not public
Deprecate old APIs (kwargs) via check!
Review doc (README)
Review examples (test-resources)

QA plan

git-status-formatter, branch-formatter, project-formatter all keep working
The examples work as advertised, customizing things as its code shows

Author checklist

Reviewer checklist

thumbnail · 2020-04-12T19:33:00Z

src/formatting_stack/protocols/spec.clj

+(spec/def ::members (spec/and (spec/coll-of ::member)
+                              (fn [xs]
+                                (let [ids (->> xs
+                                               (map (fn ^{::speced/spec (rcomp count #{0 1})} [x]


Suggested change

(map (fn ^{::speced/spec (rcomp count #{0 1})} [x]

(map (speced/fn ^{::speced/spec (rcomp count #{0 1})} [x]

thumbnail · 2020-04-12T19:34:30Z

src/formatting_stack/protocols/spec.clj

@@ -22,6 +25,23 @@ so that final users can locate them and configure them."
      (check! (spec/keys :req-un [:formatting-stack.protocols.spec.member/id])
              x))))


(speced/def-with-doc ::member "A 'member' of the stack that does something useful: a formatter, linter or processor. 'Strategies' and 'Reporters' are not members - instead they help members accomplish their purpose." (spec/or :id (spec/keys :req-un [:formatting-stack.protocols.spec.member/id]) ;; we are facing a `reify`, which means that formatting-stack is being customized ;; In those cases, an :id is practically useless (since the point of :id is overriding f-s), so no validation needed: :reify ::reify))

This will work without calling check! / valid? in the spec.

thumbnail · 2020-04-12T19:37:45Z

src/formatting_stack/protocols/spec.clj

+                                    (assert (apply distinct? ids)
+                                            "Members should have unique ids"))


This spec should be refactored so it leverages clojure.spec more. which improves testing/debugging/extensibility

Will check what can be done. At the very least a pr-str ids is due

thumbnail · 2020-04-12T20:10:01Z

test/unit/formatting_stack/impl/overrides.clj

+                                                    (try
+                                                      (sut/apply-overrides members overrides)
+                                                      (catch AssertionError _
+                                                        ::EXCEPTION))))


Why not rethrow this exception? Currently it'll swallow the error.

There's an are test case that will expect this ::EXCEPTION, so these aren't omitted

(that's part of why I put these in caps - to make it more immediately visible that something is both declared and consumed)

Could we split that are up, so the ones expecting an exception can use thrown-with-msg? for a more specific test? (e.g. now any assertionerror will pass)

Maybe I wouldn't split the are- a single are means more contrast, less apples-vs-oranges etc

Will look into making this are itself more accurate, should be possible

thumbnail · 2020-04-12T20:11:22Z

src/formatting_stack/branch_formatter.clj

                                   :in-background? in-background?)))

-(defn lint-branch! [& {:keys [target-branch in-background? reporter]
+(defn lint-branch! [& {:keys [target-branch in-background? reporter overrides]


Why nest formatters/linters/processors/strategies under override? They seem direct arguments for lint-branch!

The first thing that comes to mind is that the config should be a single map, so that it can become data.

data (a single hashmap) is simpler than code (a more intrincate invocation consisting of multiple arguments)

a single hashmap can be (more) easily a resource, a piece of Lein config, etc.

this is important, as one will want to share config across projects.

I envision this being accomplished by creating a single dep, containing a .edn as a resource. Zero config or requires to be done for consumers: we just discover the presence of such a config file.

Hashmaps can be (deep) merged

Not hard to imagine that people can want a merge of f-s defaults <-> workplace defaults <-> personal defaults.

It's also good to emphasize the notion of overrides as:

a thing you should be aware of

if I can pass :linters separately, I can be unaware of the existence a deep-merged overrides system

unawareness -> confusion, surprises, etc

the sole intended API

More APIs -> more complexity (surface area for bugs)

thumbnail · 2020-04-12T20:13:49Z

src/formatting_stack/core.clj

+                         {formatter-overrides                 :formatters
+                          linter-overrides                    :linters
+                          processor-overrides                 :processors
+                          {formatters-strategies :formatters
+                           linters-strategies    :linters
+                           processors-strategies :processors} :strategies} :overrides}]


Can we deal with the overrides one level higher e.g. create a fn which applies overrides, then call format!?

That way format! does less, and doesn't have to change shape, except for the removal of defaults (which is a good thing imo)

thumbnail · 2020-04-12T20:18:27Z

src/formatting_stack/core.clj

-                         processors
-                         reporter
-                         in-background?]}]
+(speced/defn format! [& {:keys                                             [^vector? strategies


Not a big fan of the factory-concept here. We could assoc the strategies in format! (they are impl detail anyway), which would remove the need of creating the formatters/etc here, retaining the vectors as params.

third-party-indent-specs is still an issue, suggestion posted below

We could assoc the strategies in format! (they are impl detail anyway)

See my comment beginning with The first thing that comes to mind [...]

thumbnail · 2020-04-12T20:23:07Z

src/formatting_stack/git_status_formatter.clj

+
+(speced/defn default-formatters [_
+                                 ^vector? formatters-strategies
+                                 ^map? third-party-indent-specs]


Let's add a default for third-party-indent-specs in cljfmt/new and cider/new. Then we can remove the references to third-party-indent-specs.

Consumers should overwrite the entire member if they want to change the specs. (which is how other members should work anyway). wdyt?

thumbnail · 2020-04-12T20:26:13Z

src/formatting_stack/git_status_formatter.clj

+              (assoc :strategies (conj formatters-strategies
+                                       strategies/files-with-a-namespace
+                                       strategies/exclude-edn)))
+          (when (strategies/refactor-nrepl-available?)


Can we solve this with override-configuration? e.g. adding the formatter when nrepl is available.

vemv · 2020-05-24T20:01:21Z

src/formatting_stack/branch_formatter.clj

+(defn format-and-lint-branch!
+  "Note that files that are not completely staged will not be affected.
+
+  You can override that with `:blacklist []`."


The docstring is correctly reflects the intended behavior, however not linting unstaged files is suboptimal.

After all, linting things is safe, so there's no reason to omit it (and it tends to be surprising; has bit me in the past).

Should be easy to fix.

vemv · 2020-05-24T20:20:10Z

src/formatting_stack/branch_formatter.clj

                                   :in-background? in-background?)))

-(defn lint-branch! [& {:keys [target-branch in-background? reporter]
+(defn lint-branch! [& {:keys [target-branch in-background? reporter overrides]


The first thing that comes to mind is that the config should be a single map, so that it can become data.

data (a single hashmap) is simpler than code (a more intrincate invocation consisting of multiple arguments)

a single hashmap can be (more) easily a resource, a piece of Lein config, etc.

this is important, as one will want to share config across projects.

I envision this being accomplished by creating a single dep, containing a .edn as a resource. Zero config or requires to be done for consumers: we just discover the presence of such a config file.

Hashmaps can be (deep) merged

Not hard to imagine that people can want a merge of f-s defaults <-> workplace defaults <-> personal defaults.

It's also good to emphasize the notion of overrides as:

a thing you should be aware of

if I can pass :linters separately, I can be unaware of the existence a deep-merged overrides system

unawareness -> confusion, surprises, etc

the sole intended API

More APIs -> more complexity (surface area for bugs)

vemv · 2020-05-24T20:21:06Z

src/formatting_stack/branch_formatter.clj

+  [& {:keys [target-branch in-background? reporter formatters blacklist overrides]
+      :or   {target-branch  "master"
+             in-background? (not (System/getenv "CI"))
+             reporter       default-reporter}}]


We could :pre that no :linters, :formatters etc are passed as an argument (b/c deprecation)

vemv · 2020-05-24T20:22:25Z

src/formatting_stack/core.clj

-                         processors
-                         reporter
-                         in-background?]}]
+(speced/defn format! [& {:keys                                             [^vector? strategies


We could assoc the strategies in format! (they are impl detail anyway)

See my comment beginning with The first thing that comes to mind [...]

vemv · 2020-05-24T20:27:13Z

src/formatting_stack/protocols/spec.clj

+                                    (assert (apply distinct? ids)
+                                            "Members should have unique ids"))


Will check what can be done. At the very least a pr-str ids is due

vemv · 2020-05-24T20:29:07Z

test/unit/formatting_stack/impl/overrides.clj

+                                                    (try
+                                                      (sut/apply-overrides members overrides)
+                                                      (catch AssertionError _
+                                                        ::EXCEPTION))))


There's an are test case that will expect this ::EXCEPTION, so these aren't omitted

(that's part of why I put these in caps - to make it more immediately visible that something is both declared and consumed)

vemv · 2021-09-17T20:51:10Z

Let's try the alternative approach that is in the oven 👀

vemv added 7 commits April 2, 2020 01:32

Introduce :overrides API

d5540f9

Adapt formatting-stack.branch-formatter

b2a42b4

Adapt formatting-stack.project-formatter

86b470f

Introduce Factory protocols

019b1db

Adapt Component, Integrant

6d77417

Update examples

873f054

Accept :overrides arguments in remaining functions

c2e3259

thumbnail reviewed Apr 12, 2020

View reviewed changes

vemv mentioned this pull request Apr 25, 2020

f-s must be run binding *read-eval* to false #158

Open

vemv mentioned this pull request May 4, 2020

implement lint! for formatters #154

Merged

28 tasks

vemv commented May 24, 2020

View reviewed changes

vemv mentioned this pull request Feb 23, 2021

clj-kondo improvements #174

Closed

26 tasks

vemv closed this Sep 17, 2021

thumbnail deleted the 134-deep-merge branch October 8, 2021 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `:overrides` option #152

Introduce `:overrides` option #152

vemv commented Apr 1, 2020 •

edited

Loading

thumbnail Apr 12, 2020

thumbnail Apr 12, 2020

thumbnail Apr 12, 2020

vemv May 24, 2020

thumbnail Apr 12, 2020

vemv May 24, 2020

thumbnail May 25, 2020

vemv May 28, 2020

thumbnail Apr 12, 2020

vemv May 24, 2020

thumbnail Apr 12, 2020

thumbnail Apr 12, 2020

vemv May 24, 2020

thumbnail Apr 12, 2020 •

edited

Loading

thumbnail Apr 12, 2020 •

edited

Loading

vemv May 24, 2020

vemv May 24, 2020

vemv May 24, 2020

vemv May 24, 2020

vemv May 24, 2020

vemv May 24, 2020

vemv commented Sep 17, 2021

	(map (fn ^{::speced/spec (rcomp count #{0 1})} [x]
	(map (speced/fn ^{::speced/spec (rcomp count #{0 1})} [x]

		@@ -22,6 +25,23 @@ so that final users can locate them and configure them."
		(check! (spec/keys :req-un [:formatting-stack.protocols.spec.member/id])
		x))))

		(assert (apply distinct? ids)
		"Members should have unique ids"))

Introduce :overrides option #152

Introduce :overrides option #152

Conversation

vemv commented Apr 1, 2020 • edited Loading

Brief

TODO

QA plan

Author checklist

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thumbnail Apr 12, 2020 • edited Loading

Choose a reason for hiding this comment

thumbnail Apr 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv commented Sep 17, 2021

Introduce `:overrides` option #152

Introduce `:overrides` option #152

vemv commented Apr 1, 2020 •

edited

Loading

thumbnail Apr 12, 2020 •

edited

Loading

thumbnail Apr 12, 2020 •

edited

Loading