-
Notifications
You must be signed in to change notification settings - Fork 69
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
Support inline whitelisting for :require/:imports used for side-effects #270
Support inline whitelisting for :require/:imports used for side-effects #270
Conversation
Let me know if you're happy with this approach, if so, I can add dox about it as well |
This could also be used as a workaround for #194 |
Hi, thanks for giving a damn and opening a PR! 💯 We've had a similar PR open for discussion a while back and at that point I made the note that instead of In thinking about this now, though, I think this might be the wrong solution to the problem of side-effecting namespaces getting pruned from the ns. My thinking is that we probably don't want to get into the business of adding tooling-specific metadata to source code. On my team we have three different editors in use, can you imagine if each of these tools took the same approach? (ns resources.ns-with-whitelisted-deps
(:require [^:refactor-nrepl/side-effects ^:orchard/keep :^cursive/side-effecting ^:blessed clojure.set :as set]
[clojure.string :as str])
(:import java.sql.Timestamp
^:refactor-nrepl/side-effects ^:orchard/keep :^cursive/side-effecting ^:blessed java.util.Date)) clj-condo recently had the same discussion and landen on a solution they were happy with. Unfortunately, that solution only applies to I think we should stick with solving this using the config options (which can be combined with dir-local settings so every emacs user on a project doesn't have to update their settings). With a slightly broader audience in mind, perhaps Orchard might at some point get a per-project config file that we can piggy-back on. As to your particular problem in your project, @camsaul, you can configure the call to |
How about not removing required namespaces with no alias and no referrals?
…On Tue, Nov 5, 2019 at 3:53 PM Lars Andersen ***@***.***> wrote:
Hi, thanks for giving a damn and opening a PR! 💯
We've had a similar PR open for discussion a while back and at that point
I made the note that instead of ^:keep we should namespace the keyword as
:refactor-nrepl/keep so team members using other tooling had some way to
google their what way to finding out what in the hell this metadata does.
In thinking about this now, though, I think this might be the wrong
solution to the problem of side-effecting namespaces getting pruned from
the ns. My thinking is that we probably don't want to get into the business
of adding tooling-specific metadata to source code. On my team we have
three different editors in use, can you imagine if each of these tools took
the same approach?
(ns resources.ns-with-whitelisted-deps
(:require [^:refactor-nrepl/side-effects ^:orchard/keep :^cursive/side-effecting ^:blessed clojure.set :as set]
[clojure.string :as str])
(:import java.sql.Timestamp
^:refactor-nrepl/side-effects ^:orchard/keep :^cursive/side-effecting ^:blessed java.util.Date))
clj-condo recently had the same discussion
<clj-kondo/clj-kondo#241> and landen on a
solution they were happy with. Unfortunately, that solution only applies to
require statements and therefore only solves half the problem.
I think we should stick with solving this using the config options (which
can be combined with dir-local settings so every emacs user on a project
doesn't have to update their settings). With a slightly broader audience in
mind, perhaps Orchard might at some point get a per-project config file
that we can piggy-back on.
As to your particular problem in your project, @camsaul
<https://github.com/camsaul>, you can configure the call to clean-ns by
wrapping it with (refactor-nrepl.config/with-config {:libspec-whitelist
["java.util.Date" ...]}...) More details about the available settings in
the readme.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#270?email_source=notifications&email_token=AACA4OIASPSUNPIH72XFZQDQSGCIHA5CNFSM4JI2SV2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDC6TOY#issuecomment-549841339>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACA4OIPF6PUP757CWJH5CDQSGCIHANCNFSM4JI2SV2A>
.
|
This is how clj-condo solved it, but that leaves side-effecting imports in the lurch. |
Agree on not having tooling-specific metadata. I also work on a team with different editors.
This starts to reduce the utility of Other times, the required namespace consists of only a root name, for example timbre-ns-pattern-level. All the proposed solutions on this thread have some kind of inelegance, and I would suggest that they be weighed against the inelegance of existing workarounds. My current workaround is use the side-effecting namespace somehow, for example:
It only adds one line to the code, makes it pretty clear that there are side effects, and should work across editors. One challenge I run into is if the side-effecting namespace has no vars, for example tick.timezone. For this I resort to an uglier workaround:
But I could imagine a future where this no longer works because ns usages inside comments no longer prevent cleaning. So my question for this discussion is: rather than metadata, is there a simple form that can be placed at the top level which involves a required namespace to prevent its cleaning? And which works even if the required ns has no vars in it? If there is such a form, then maybe that's a reasonable solution to this problem. |
The language already has such a form, thankfully. Lately I've just been putting an explicit (ns my-ns
(:require [clojure.string :as str]))
;; This has some really sweet side-effects that we want, because reasons
(require 'tick.timezone) This is something I've started doing to help the others on my team (even though I have the right variables set up to avoid accidental pruning), and it has a few benefits:
|
That’s a really neat solution. 👍
ons. 6. nov. 2019 kl. 14:16 skrev Lars Andersen <notifications@github.com>:
… If there is such a form, then maybe that's a reasonable solution to this
problem.
The language already has such a form, thankfully.
Lately I've just been putting an explicit (import ...) or (require ...),
with a comment about the desired side-effects, on its own line below the
ns form. e.,g
(ns my-ns
(:require [clojure.string :as str]))
;; This has some really sweet side-effects that we want, because reasons
(require 'tick.timezone)
This is something I've started doing to help the others on my team (even
though I have the right variables set up to avoid accidental pruning), and
it has a few benefits:
1. It clearly shows that this import / require is special and requires
some extra attention.
2. Easily understandable to anyone
3. Doesn't require any out-of-band communication (e.g. you don't have
to know about tooling metadata or conventions with additional meanings like
[this-will-be.left-alone])
4. Looks better than adding a comment in the ns form anyway, that may
or may not be removed by tooling
5. Robust (we've lost important side-effecting imports to tooling bugs
/ eager janitors).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#270?email_source=notifications&email_token=AACA4OLKWNKFR2YRCWEKBKLQSK7UHA5CNFSM4JI2SV2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDGPUSA#issuecomment-550304328>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACA4OKJPGOLIZN5XS4A4JLQSK7UHANCNFSM4JI2SV2A>
.
|
I love it! I would be happy to write all my code this way and continue to use But does it work in clojurescript? The docs say "Only usable from a REPL". |
My strategy in the past has been to add a separate top-level
Another example is one of Eastwood's linters which checks for referencing vars without requiring their namespace first. This linter also only looks at the I'll have to look at either using a combo of a global config and |
@camsaul thanks for pointing this out. In my case, I use tools.namespace, which I just checked and also only looks at the top-level Well it's a kludge, but for now I think I'm going to write:
I wish there were a better form to put in the body, but I can't think of one. |
So, how are we going to proceed here? I don't quite remember what kondo opted for, but I guess we should take the same approach here. |
I'm in favor of just closing this, because there's no good way of solving this atm. In summary: the practice I suggested above works really well for clj, but not at all for cljs. The clj-condo approach doesn't work for imports, and also depends on educating users that they can get a certain outcome if they adopt a certain code style. |
fwiw, I agree with this. |
I am hitting this as well, in particular with specs I require files which contain specs so that they are loaded into the global registry but running clean will remove the require and break the code. Just pointing this out as no one mentioned spec specifically as an issue, I like the idea of the meta data flag ^refactor-skip or something. going to use the comment hack for now. |
Closing this in favor of this issue and enabling opt-in support for the clj-kondo approach behind a setting. clj-kondo is great and it's usage has been increasing steadily. Over time I expect it to be the dominant linter (if it isn't already). At that point the argument about the need to educate users goes out the window (since they do that for us and have a much larger user base). If we make it opt-in it would only affect users that want this behavior. |
Tweaks
clean-ns
so that:require
and:import
symbols with^:side-effects
metadata won't be removed. This is useful for cases where want import a namespace for side effects without havingclean-ns
remove it, or for false positives (such as #239).before
after calling
clean-ns
Setting
cljr-libspec-whitelist
, the workaround mentioned in #239, doesn't work when running outside of Emacs – for example when using this namespace declaration linter I put together usingclean-ns
.To make this easier to use correctly it accepts
^:side-effects
on any of the following::require
/:import
namespace/class symbol itself, e.g.(:require ^:side-effects clojure.string)
(:require ^:side-effects [clojure data string])