bowbahdoe / caveman

Clojure Web Framework
https://caveman.mccue.dev
12 stars 8 forks source link

Replace `UUID/fromString` with `parse-uuid` #11

Closed souenzzo closed 3 weeks ago

souenzzo commented 3 weeks ago

We have 10 occurrences of UUID/parseString, that requires a few import (java.util UUID)

Advantages of moving to parse-uuid

potential problems

What you guys thing about this?

If I have support from the community, I can investigate possible issues and make changes.

bowbahdoe commented 3 weeks ago

Oh - i'll be honest I just didn't remember/know that parse-uuid was added.

I'll think a little on the implications of nil versus an exception, but my guess is it'll be fine.

bowbahdoe commented 3 weeks ago

Change made

bowbahdoe commented 3 weeks ago

Took a long shower and realized I actually prefer the behavior of UUID/fromString. In all the contexts it was used getting an exception is actually preferable to nil on error.

Re-opening issue for folks to try to convince me otherwise

souenzzo commented 3 weeks ago

OK I will try to convince you that even if you prefer this behavior in this function, in most functions, you haven't even thought about it.

For example, in this parse long, if you had a long shower thinking about it, I think that probably you would decide for Long/parseLong, that throws on invalid strings

But probably, rich hickey took a long shower, and decided that clojure.core parse functions should return nil.

bowbahdoe commented 3 weeks ago

in most functions, you haven't even thought about it.

That is true.

bowbahdoe commented 3 weeks ago

I went ahead and changed that to Long/parseLong.

I don't object to the design of the core library ones, but I do want fail-faster behavior and don't really care about CLJS in this context, so I see no reason to not use the Java ones. The import doesn't bother me either. Files can have a lot of imports.