babashka / neil

A CLI to add common aliases and features to deps.edn-based projects
MIT License
367 stars 27 forks source link

Issue 170 dan #172

Closed dmg46664 closed 1 year ago

dmg46664 commented 1 year ago

Not sure this is the right fix, could be that you want to guarantee the semver map is ints to start with.

borkdude commented 1 year ago

Thanks, this is very helpful.

Somehow in my first commit, something was turned into strings. Maybe it's good to find out why that happened.

Also Integer. can now be written as parse-long in bb and clojure :-)

dmg46664 commented 1 year ago

@borkdude Good to know. The following should probably be updated then? https://clojuredocs.org/clojure.core/cond-%3E#example-5be9f0e6e4b00ac801ed9ef2

borkdude commented 1 year ago

Yes, there is an open PR for clojure 1.11

dmg46664 commented 1 year ago

Is it a warning in clj-kondo 🤔 😅 ?

borkdude commented 1 year ago

What do you mean? Only if you have an older version of clojure on your classpath, but normally clj-kondo does not warn on that.

dmg46664 commented 1 year ago

I'm just teasing. I mean if you are using Integer. instead of parse-long... or were we talking at cross purposes?

borkdude commented 1 year ago

I thought you meant clj-kondo warned you about parse-long. But yes, that might be nice :)

dmg46664 commented 1 year ago

@borkdude Ah, you merged it. I probably should have said I was thinking about doing that investigating you suggested (I just didn't want to commit to it, so didn't write it). Anyway, thanks for accepting it regardless. 🙏

borkdude commented 1 year ago

Feel free to do this after the fact.

borkdude commented 1 year ago

I'll wait a bit with releasing a new version.

dmg46664 commented 1 year ago

@borkdude Okay, the reason the test failed is because: (neil "version minor 4 :no-tag" :out :string) used to pass the opt of { ... :version 4 } as in to say override version with the value 4. Because of the change :coerce :string, the value for :version is now a string! Therefore the fix is correct, unless you want a different spec for :version in the overriding :minor case. IMO that's a little pedantic for this use case given the single catch all babashka.neil/spec.

borkdude commented 1 year ago

Thanks!

On Tue, 7 Mar 2023 at 02:52, Daniel Gerson @.***> wrote:

@borkdude https://github.com/borkdude Okay, the reason the test failed is because: (neil "version minor 4 :no-tag" :out :string) used to pass the opt of { ... :version 4 } as in to say override version with the value 4. Because of the change :coerce :string, the value for :version is now a string! Therefore the fix is correct, unless you want a different spec for :version in the overriding :minor case. IMO that's a little pedantic for this use case given the single catch all babashka.neil/spec.

— Reply to this email directly, view it on GitHub https://github.com/babashka/neil/pull/172#issuecomment-1457366134, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACFSBQL63ZPOARNGD7XM43W22IHXANCNFSM6AAAAAAVRJQT7M . You are receiving this because you were mentioned.Message ID: @.***>

-- https://www.michielborkent.nl https://www.eetvoorjeleven.nu