fix reload-all and test

[filipesilva]

Before submitting a PR make sure the following things have been done:

• [x] The commits are consistent with our contribution guidelines
• [x] You've added tests to cover your change(s)
• [ ] All tests are passing
• [ ] You've updated the README
• [ ] Middleware documentation is up to date
  • Please check out and modify the cider.nrepl ns which has all middleware documentation.
  • Run lein docs afterwards, and commit the results.

Note: If you're just starting out to hack on cider-nrepl you might find nREPL's documentation and the "Design" section of the README extremely useful.*

Thanks!

[filipesilva]

I see some failing but they seem like a connectivity issue when testing against clojure 1.12?

Retrieving mx/cider/logjam/0.3.0/logjam-0.3.0.jar from clojars
Could not find artifact org.clojure:clojure:jar:1.12.0-master-SNAPSHOT in clojars (https://repo.clojars.org/)
Could not transfer artifact org.clojure:clojure:jar:1.12.0-master-SNAPSHOT from/to snapshots (https://oss.sonatype.org/content/repositories/snapshots): transfer failed for https://oss.sonatype.org/content/repositories/snapshots/org/clojure/clojure/1.12.0-master-SNAPSHOT/clojure-1.12.0-master-SNAPSHOT.jar, status: 502 Bad Gateway
Failed to read artifact descriptor for org.clojure:clojure:jar:1.12.0-master-SNAPSHOT
This could be due to a typo in :dependencies, file system permissions, or network issues.
If you are behind a proxy, try setting the 'http_proxy' environment variable.
Error encountered performing task 'install' with profile(s): 'base,system,provided,master,config'
Could not resolve dependencies
make[1]: *** [Makefile:116: install] Error 1
make[1]: Leaving directory '/root/repo'
make: *** [Makefile:129: smoketest] Error 2

[vemv]

Thanks!

While we're here, please replicate the before/after function calling https://github.com/clojure-emacs/cider-nrepl/blob/95ce24bf1f40ef566aed2d073a41c943723cf1bd/src/cider/nrepl/middleware/refresh.clj#L64

It is ok to make some defn in cider.nrepl.middleware.refresh public so that they can be shared across these two namespaces.

[filipesilva]

done

[vemv]

You're fast!

Please update the changelog and it's ready to merge

[bbatsov]

I'm not sure I like introducing dependencies between middlewares, so I'd rather extract the functions they need to share out of the refresh middleware. While the current approach works, it forces us to load 2 middleware namespaces even if the users will ever call into one, which is suboptimal IMO. (especially when talking about a namespace with a heavy dep like tools.namespace)

[bbatsov]

Thanks!

While we're here, please replicate the before/after function calling

https://github.com/clojure-emacs/cider-nrepl/blob/95ce24bf1f40ef566aed2d073a41c943723cf1bd/src/cider/nrepl/middleware/refresh.clj#L64

It is ok to make some defn in cider.nrepl.middleware.refresh public so that they can be shared across these two namespaces.

I already responded above, but I think that's a bad approach, so let's not do it. I'd prefer to move the code to some shared util-type namespace.

[filipesilva]

@vemv I think it's better if you move this code yourself. Like I said in the other PR, I wasn't really aiming to add this functionality in the first place, and now it will involve some non trivial code refactor that I think will be subject to more back and forth. I really don't want to go into a 4th weekend working on this.

[filipesilva]

Removed the commit that was adding functionality and left just the code fix and test for reload-all.

[bbatsov]

@filipesilva That's completely fair. Just remove this change from the PR and we're good to go.

[bbatsov]

Removed the commit that was adding functionality and left just the code fix and test for reload-all.

Don't forget to mention the bug-fix here https://github.com/clojure-emacs/cider-nrepl/blob/master/CHANGELOG.md

[filipesilva]

Ah right, I forgot that bit. Added now.

[bbatsov]

We'll cut a new release with your fix soon.