search

weavejester / medley

A lightweight library of useful Clojure functions
Eclipse Public License 1.0
865 stars 66 forks source link

Add cljr support #65

Closed E-A-Griffin closed 1 year ago

E-A-Griffin commented 2 years ago

Adds ClojureCLR support for source/test files, packaging/dependency management, and CI testing.

NoahTheDuke commented 2 years ago

I wonder if, given the similarity between the clj and cljr branches, instead of listing both, you should use :default as the catch all.

E-A-Griffin commented 2 years ago

I had considered using :default where there was common overlap between :clj and :cljr but I wasn't sure what best practice was. If @weavejester prefers :default for overlap between clj and cljr then I can certainly make those changes.

weavejester commented 2 years ago

I had considered using :default where there was common overlap between :clj and :cljr but I wasn't sure what best practice was. If @weavejester prefers :default for overlap between clj and cljr then I can certainly make those changes.

The reader conditionals page on the Clojure website seems to suggest that :default is designed to be a fallback, and therefore it would seem like the best choice is to be explicit and use :clj and :cljr even in cases where the code is identical.

weavejester commented 1 year ago

Hey there, sorry for the long wait. I've only recently been able to get back to reviewing Clojure PRs and I've been working my way through the list. I notice that in this PR the csproj and sln file seem to have been copied rather than moved? Other than that it looks fine.

E-A-Griffin commented 1 year ago

Hey there, sorry for the long wait. I've only recently been able to get back to reviewing Clojure PRs and I've been working my way through the list. I notice that in this PR the csproj and sln file seem to have been copied rather than moved? Other than that it looks fine.

I must not have pushed the local change removing those files. Everything should be good now, thanks for the re-review!

weavejester commented 1 year ago

Again, apologies for letting this lie for a while. I've been hesitant to merge this, because in doing so I obviously commit to maintaining an additional language target for Medley, and I've been unwilling to commit one way or the other. On the one hand your work is excellent; on the other I don't think I can reasonably maintain this.

After thinking it through, my thought is that I don't merge this into Medley, and instead I ask you to set up a separate repository for it. I can link to the new repo through Medley's README, so people can easily find the CLR version, but it wouldn't be something I'd maintain or work on. Again, I really appreciate the work you've put into this, but I don't think I'm the person to maintain it.

E-A-Griffin commented 1 year ago

No worries on the wait. Yeah, I understand the position you're in, especially given the volume of popular repositories you maintain for the Clojure community as it is. It does make me a bit sad to read, but logistically I get it.

If that's the course of action you want to take, then maybe just link to the current fork of medley I have, i.e. https://github.com/E-A-Griffin/medley, in the README and I can maintain that.

weavejester commented 1 year ago

Thank you for understanding. I've added a link to the README to your repository.