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

default to all dirs #4

Closed
wants to merge 1 commit into from
Closed

default to all dirs #4

wants to merge 1 commit into from

Conversation

filipesilva
Copy link

This default means that a user does not have to init before running reload. But they can.

Mimics tools.namespace behaviour.

@filipesilva filipesilva marked this pull request as draft February 23, 2024 11:51
This default means that a user does not have to init before running reload. But they can.

Mimics [tools.namespace behaviour](https://github.com/clojure/tools.namespace/blob/05328ed700840165bcf1c5d34d5ef2e299b58c1a/src/main/clojure/clojure/tools/namespace/dir.clj#L91).
@vemv
Copy link

vemv commented Feb 23, 2024

My 2c (and even if it means looking like a party pooper 😇) would be that in this particular instance, clj-reload's strictness is better than t.n's laxity.

In specific terms, it's not rare to have .clj files in directories that aren't meant to be code-loaded e.g.:

  • test-resources/ (maybe your test suite exercises some intentionally broken .clj samples)
  • target/classes (maybe the build process ends up moving .clj files there for whatever reason, then those files can create hard-to-debug issues)

I've seen that kind of thing over the years, and also various people struggle with t.n because of the lack of explicit setup, so it seems a good chance to start with a cleaner slate?

@filipesilva
Copy link
Author

I haven't been faced with those issues so it's pretty good input to know it happens.

In these cases, was it that users were surprised tools.namespace didn't "just work" and that they had to do some setup? I mean, it seems the "information" needed for them to know was available (codebase-specific test files, build steps), but on a large enough codebase it shouldn't be a surprise people know all of it.

@vemv
Copy link

vemv commented Feb 23, 2024

From memory, most times people weren't aware of the notion of refresh-dirs existed, therefore they were unset, and they didn't understand why something went wrong.

I'd go as far as saying that some people thought that t.n's was flawed by design, e.g. it loads whatever code it finds in the classpath. This had given t.n sort of a mixed fame.

@vemv
Copy link

vemv commented Feb 23, 2024

...it might make sense to honor a System property to set the dirs to :all or ["src" "foo" "bar"], which would lead to setups that can be written just once (managed via ~/.lein / ~/.clojure), and less obstrusive (not version-controlled).

But I wouldn't recommend changing the default behavior to something more lax.

@filipesilva
Copy link
Author

Worth mentioning that effect seems to be some of the point of clj-reload: to be the "righter" thing out of the box, allow on-var configuration to not reload (via ^:clj-reload/keep), and setting unload hooks as before-ns-unload named fns directly on the ns.

None of those things require importing clj-reload. So they enable interesting usage patterns where the codebase does not need to include the dependency. It is enough for editor tooling or user-specific config to add it.

@tonsky tonsky closed this in 0a082e9 Feb 23, 2024
@tonsky
Copy link
Owner

tonsky commented Feb 23, 2024

I think being able to set clj-reload once system-wide and then use it everywhere without explicitly adding it to the project is not a bad idea. I still prefer explicit init and not putting meta-tags in production code, but why not have it as an option?

Implemented in 0a082e9 and published as 0.2.0

Thanks for the idea, @filipesilva

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

Successfully merging this pull request may close these issues.

3 participants