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

duplicated defaults #134

Open
thumbnail opened this issue Feb 24, 2020 · 6 comments
Open

duplicated defaults #134

thumbnail opened this issue Feb 24, 2020 · 6 comments
Assignees
Labels

Comments

@thumbnail
Copy link
Member

Brief

defaults are duped over https://github.com/nedap/formatting-stack/blob/master/src/formatting_stack/defaults.clj, https://github.com/nedap/formatting-stack/blob/master/src/formatting_stack/branch_formatter.clj and https://github.com/nedap/formatting-stack/blob/master/src/formatting_stack/project_formatter.clj with minor (but existing) differences.

Expected behavior

All defaults live in https://github.com/nedap/formatting-stack/blob/master/src/formatting_stack/defaults.clj

Actual behavior

defaults are duped

Suspected cause

differences in configuration between core/project-formatter/branch-formatter

Environment info

<= v4.0.0

Additional links

Thanks to #130 we can now apply the configuration differences needed in project-formatter/branch-formatter to the defaults.

@vemv
Copy link
Contributor

vemv commented Feb 24, 2020

Maybe the protocols could some new methods:

(defprotocol Formatter (default-strategies [_] "The default strategies that this forrmatter tends to need"))

That way the duplication would be killed. One still should be able to override the default behavior.

@thumbnail
Copy link
Member Author

Maybe the protocols could some new methods:

Interesting approach. Some linters still do some filtering themselves (remove edn files, unrequireable files, blacklisting known-bad files, etc.). Maybe it's more suited as part of #37

That way the duplication would be killed.

The simplest approach would be to remove the defaults in project-formatter and branch-formatter, and depend on the defaults in formatting-stack.defaults, overriding the defaults where necessary.

@vemv
Copy link
Contributor

vemv commented Feb 25, 2020

Traditionally I've seen this as a non-issue since "it's just data".

However I'm starting to see the value/need for this, as I've tried to customize heavly one real project. Some parts could be customized as the examples advertise; others needed more knowledge of the f-s impl.

At the same time it's quite crucial to keep f-s simple/easy (for us and for any casual user). I'd be wary of some DSL, hooks, DAG libs, etc

e.g. an API resembling a deep-merge would seem simple:

{:in-background? false
 :overrides {:linters {::kondo/id {:kondo-clj-options {:linters {:cond-else {:level :warning}}}}}
             :formatters {::newlines/id nil ;; remove it
                          ::cljfmt/id {:third-party-indent-specs {`a :defn}}}}}

As you pointed out somewhere, the :id addition enables this sort of stuff.


Prior to any POC, please let's seek agreement + assignment first.

@thumbnail
Copy link
Member Author

On slack we discussed using a map for the configuration, but decided against that because of the implicit ordering it brings. Your current suggestion 'works around' that problem by only changing the config using a map, but a vector upon initialization, right?

Is this a similar approach as initially suggested in #38 ?


I like the overall approach, nice stuff 👍

@vemv
Copy link
Contributor

vemv commented Feb 25, 2020

Your current suggestion 'works around' that problem by only changing the config using a map, but a vector upon initialization, right?

Correct - vectors (:formatters, :linters etc) are handled, and a single hashmap can override specific aspects of those vectors.

Is this a similar approach as initially suggested in #38 ?

Not sure, as I didn't thought #38 much in advance. I had just thought "supdate seems nice"

@vemv vemv added core and removed low-priority labels Mar 2, 2020
@vemv vemv self-assigned this Mar 2, 2020
@vemv
Copy link
Contributor

vemv commented Mar 2, 2020

Taking over this one, seems easy and quite important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants