merge command line args

jonase / eastwood

Clojure lint tool

1.09k stars 66 forks source link

merge command line args #434

Closed brettatoms closed 2 years ago

brettatoms commented 2 years ago

Currently eastwood doesn't correctly parse command line arguments if there is already a configuration in deps.edn. For example if we have this in ours deps.edn:

{:aliases {:eastwood
           {:main-opts ["-m" "eastwood.lint" {}]
            :extra-deps {jonase/eastwood {:mvn/version "1.2.3"}}}}}

Then running clojure -M:eastwood "{:namespaces [user]}" won't check only the user namespace.

This PR allows the opts to be a list of any combination of maps or strings and merges them with the default opts to be passed to eastwood.lint/eastwood-from-cmdline

Before submitting the PR make sure the following things have been done (and denote this by checking the relevant checkboxes):

[ ] You've updated the changelog (if adding/changing user-visible functionality)
[ ] You've updated the readme (if eg adding a new linter)

Thanks!

vemv commented 2 years ago

I've reflected a bit on the use case and I preferred to prevent creating something that can become easily complex (think precedence rules, etc).

Eastwood config is not supposed to change a lot, so overriding config is not generally justified.

"Run only this namespace" can be accomplished via other means. https://github.com/jonase/eastwood/issues/432 is a good read.

Worst-case scenario, you can create an additional deps.edn alias that lacks the {} so it's ready for being specified from the CLI.

Feel free to open an issue or to simply continue the conversation here.

brettatoms commented 2 years ago

At the very least I thought this PR was doing what (->> opts (interpose " ") (apply str) (edn/read-string)) was trying to do. At the moment the expression (->> ["{:a 1}" "{:b 2}"] (interpose " ") (apply str) (clojure.edn/read-string)) evaluates to {:x 1}. Was that the intention?

vemv commented 2 years ago

The related commit is https://github.com/jonase/eastwood/commit/4d1d05cc6de6ce565207d9a3c86864364f53f006 which doesn't seem concerned with merging maps - only with map vs. string compatibility?

In absence of unit tests for said commit, I'd be happy to consider the current overall behavior correct (since I've had no problems with real-world deps.edn usage in my own work projects), and "merge" behavior to be undefined.

Of course we can eventually improve things as hinted in https://github.com/jonase/eastwood/pull/434#issuecomment-1106620436 .