sailfish icon indicating copy to clipboard operation
sailfish copied to clipboard

Impl Render for NaiveDateTime

Open nerdunit opened this issue 2 years ago • 6 comments

Are there any plans to add optional features like for chrono that would implement Render for NaiveDateTime?

nerdunit avatar Aug 07 '23 19:08 nerdunit

Check my pr.

mvntainer avatar Aug 09 '23 16:08 mvntainer

@mvntainer Do you have anything I could add to the ReadMe, or anything about how to use this changeset? I am looking it over now, and would love to hear your guidance for how best to use this new code. If you can give me the gist of it, I can add it to our documentation to make things easier for new developers.

vthg2themax avatar Aug 10 '23 13:08 vthg2themax

@mvntainer Do you have anything I could add to the ReadMe, or anything about how to use this changeset? I am looking it over now, and would love to hear your guidance for how best to use this new code. If you can give me the gist of it, I can add it to our documentation to make things easier for new developers.

Sorry for the late reply, I have never made any crate and I'm not sure if how I used optional depends is 100% correct, but adding sailfish = { path = "../sailfish/sailfish", features = ["extended_support"] } will also add support for types from other crates such as chrono and uuid, you could make custom features for every crate if that's what you choose, why I decided to add this behind feature flags is because some people don't want to extend dependencies or already have compile time problems from a lot of crates, this will only download and compile chrono and uuid only if the flag is set, the code that is behind #[cfg(feature="FEATURE")], will only compile if that feature is set.

mvntainer avatar Aug 16 '23 22:08 mvntainer

@mvntainer Do you have anything I could add to the ReadMe, or anything about how to use this changeset? I am looking it over now, and would love to hear your guidance for how best to use this new code. If you can give me the gist of it, I can add it to our documentation to make things easier for new developers.

Also the printing I used in the chrono part of the code is pretty verbose, it should be changed to something more user-friendly, at the moment I only tried to "make it work" for blueprinting a project I work on. I wanted to contribute to sailfish because of it's speed, and I don't want to see this project abandoned, thanks for your contributions to this awesome crate

mvntainer avatar Aug 16 '23 22:08 mvntainer

For things like NaiveDateTime that has a wide range of ways you could want to represent it, having direct support in Sailfish would be hard. For example, the documentation for NaiveDateTime::format has a good example of such representation:

assert_eq!(dt.format("around %l %p on %b %-d").to_string(), "around 11 PM on Sep 5");

If you want the default representation of these (which @mvntainer's PR uses), you can always use the display filter:

<span><%= date | disp %></span>

<!-- The UUID support in @mvtainer's PR also works with this -->
<span><%= uuid | disp %></span>

The display filter wraps the NaiveDateTime in the Display<T> struct, and as long as T implements the Display trait, Sailfish knows how to render it out of the box. So check your third-party structs if they implement Display and if you're OK with that representation, you can use the display filter!

But if you want custom representation, it should still be possible without direct Sailfish support for these two crates since the representation structs they return from their methods also implement Display:

<!-- faster, writes directly to Sailfish output Buffer which skips an intermediate String allocation: -->
<span><%= date.format("around %l %p on %b %-d") | disp %></span>
<span><%= uuid.urn() | disp %></span>

<!-- slower, allocates intermediate String -->
<span><%= date.format("around %l %p on %b %-d").to_string() %></span>
<span><%= uuid.urn().to_string() %></span>

Another alternative is the NewType pattern, which would be good if you want to localize the date during render, based on e.g Accept-Language HTTP request header.

Svenskunganka avatar Aug 17 '23 04:08 Svenskunganka

For things like NaiveDateTime that has a wide range of ways you could want to represent it, having direct support in Sailfish would be hard. For example, the documentation for NaiveDateTime::format has a good example of such representation:

assert_eq!(dt.format("around %l %p on %b %-d").to_string(), "around 11 PM on Sep 5");

If you want the default representation of these (which @mvntainer's PR uses), you can always use the display filter:

<span><%= date | disp %></span>

<!-- The UUID support in @mvtainer's PR also works with this -->
<span><%= uuid | disp %></span>

The display filter wraps the NaiveDateTime in the Display<T> struct, and as long as T implements the Display trait, Sailfish knows how to render it out of the box. So check your third-party structs if they implement Display and if you're OK with that representation, you can use the display filter!

But if you want custom representation, it should still be possible without direct Sailfish support for these two crates since the representation structs they return from their methods also implement Display:

<!-- faster, writes directly to Sailfish output Buffer which skips an intermediate String allocation: -->
<span><%= date.format("around %l %p on %b %-d") | disp %></span>
<span><%= uuid.urn() | disp %></span>

<!-- slower, allocates intermediate String -->
<span><%= date.format("around %l %p on %b %-d").to_string() %></span>
<span><%= uuid.urn().to_string() %></span>

Another alternative is the NewType pattern, which would be good if you want to localize the date during render, based on e.g Accept-Language HTTP request header.

Thank you, this is the proper response, can something like this be added in the README?

mvntainer avatar Aug 17 '23 07:08 mvntainer

@nerdunit @mvntainer @Svenskunganka @djarb This helpful information has been added to documentation. Thank you for your input!

If you wanted something else, let me know, but otherwise I will go ahead and close this out in about a week.

vthg2themax avatar Jun 08 '24 14:06 vthg2themax