naomijub / edn-rs

[DEPRECATED]: Crate to parse and emit EDN
https://crates.io/crates/edn-rs
MIT License
81 stars 14 forks source link

Remove `Inst` and `Uuid` #122

Closed Grinkers closed 9 months ago

Grinkers commented 9 months ago

copy/paste from the bloated/out of scope discussion on #120. Because of the test refactoring in #121, please only look at the last commit here.

Also, ok on the tagged portion.

On second thought, I think we keep the tagged type so no data is lost, but remove Uuid and and Inst. Users can match against the tagged type and do what they need to do. Right now it's not correct and I don't think we want to add the dependencies to make it correct.

(clojure.edn/read-string "#uuid \"af6d8699---f442-4dfd-8b26-37d80543186b\"")
java.lang.IllegalArgumentException: UUID string too large [at <repl>:1:1]

(clojure.edn/read-string "#inst \"2020lolfoobar-07-16T21:53:14.628-00:00\"")
java.lang.RuntimeException: Unrecognized date/time syntax: 2020lolfoobar-07-16T21:53:14.628-00:00 [at <repl>:4:1]

We just parse the string, without failure. We can add it back in when/if something like uuid and chrono get into rust std or as an optional feature if there's demand.

codecov-commenter commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (831ebf6) 71.28% compared to head (bd55855) 70.63%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #122 +/- ## ========================================== - Coverage 71.28% 70.63% -0.66% ========================================== Files 8 8 Lines 867 841 -26 ========================================== - Hits 618 594 -24 + Misses 249 247 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Grinkers commented 9 months ago

It was easier to just show instead of explain.

This leaves the future open for whatever it ends up being


    pub fn to_std_uuid(&self) -> Option<Result<std::uuid>> {
        if let Self::Tagged(tag, str) = self {
            if tag == "uuid" { std::parse::<uuid>(str) } else { None }
        } else {
            None
        }
    }
Grinkers commented 9 months ago

Rebased. Thoughts?

naomijub commented 9 months ago

We are not particularly strict with rebase strategies. Specially now that your are the main maintainer

Grinkers commented 9 months ago

We are not particularly strict with rebase strategies. Specially now that your are the main maintainer

I like to keep things clean/linear.

Sorry for the poor communication. I was looking for thoughts on the PR itself and Tags. I'm going to make a fuzzer/idiomatic rust example. I'm going to leverage the fuzzer clojure uses for EDN one for one. It has tags, so for sure we don't want to remove tag support, but we don't need to support custom readers.

naomijub commented 9 months ago

Ah ok, once I'm back home I can take a look

naomijub commented 9 months ago

It was easier to just show instead of explain.

This leaves the future open for whatever it ends up being

    pub fn to_std_uuid(&self) -> Option<Result<std::uuid>> {
        if let Self::Tagged(tag, str) = self {
            if tag == "uuid" { std::parse::<uuid>(str) } else { None }
        } else {
            None
        }
    }

I do like this idea, but I would keep the current features uuid and chrono available because I know there is usage of them.

#[cfg(feature = "uuid")]
pub fn to_uuid(&self) -> Option<Result<uuid::Uuid>> { 
// ...
}

#[cfg(feature = "chrono")]
pub fn to_datetime(&self) -> Option<Result<chrono::Datetime<T>>> { 
// ...
}

What do you think @evaporei ?

Grinkers commented 9 months ago

I thought about this, but it's not really this library's job to decide which libraries and versions to use just to wrap a couple functions from another dependency. Especially with chrono. I think there's been breaking changes in the past that were pretty bad (or maybe I'm thinking of something else) that causes really annoying problems.

naomijub commented 9 months ago

@Grinkers how about adding example usage of uuid and chrono in the example folder. Then they become simple dev dependencies and we have a documented way of dealing with them

Grinkers commented 9 months ago

@Grinkers how about adding example usage of uuid and chrono in the example folder. Then they become simple dev dependencies and we have a documented way of dealing with them

We could, but it's sort of the same problem.

For example, do we pick chrono 0.4? What about the people using a different api from 0.4, like if some other dependency/database/http library uses 0.3? What about about the in-development 0.5? Many different types of uuid stuff too. I'm guessing most people probably want to match clojure. I actually don't use clojure (jvm) too much at all, so can't really comment.

I personally feel we should just leave it as an exercise for the end-user. We parse it and give them a way match against the tag and a way to get the data, how they use it is out of scope. If/when there's a std implementation, my opinion would be very different. Then we'd still have the same internal data available, but also an idiomatic rust way to handle things.

As a compromise, how about an example how to match and get the string as it is right now?