Have DateTimeWidget include the time zone in its type. #1

ggriffiniii · 2019-09-30T15:54:55-07:00

ggriffiniii commented

2019-09-30 15:54:55 -07:00

No description provided.

wathiede reviewed 2019-10-01 20:44:30 -07:00

src/widgets/datetime.rs

						
				@ -35,0 +29,4 @@

				impl<Tz> DateTimeWidget<Tz>

				where

				    Tz: TimeZone,

				    Tz::Offset: Display,

wathiede commented

2019-10-01 20:33:34 -07:00

I don't get why "Tz::Offset: Display" is used here, can you explain that?

ggriffiniii commented

2019-10-01 22:54:56 -07:00

Well I think it may not actually be necessary here (and should probably be removed), but it is necessary on the Widget impl. Short answer for why it's necessary there is because the compiler said so. Longer answer is because the Widget impl formats the time, which requires displaying the timezone which can only be done if the timezone's offset has a Display impl.

src/widgets/datetime.rs

						
				@ -40,3 +40,2 @@

				    }

				    pub fn set_colors(&mut self, colors: &[TimeColor]) {

				        let mut colors = colors.to_vec();

				    pub fn set_colors(&mut self, colors: impl Into<Vec<TimeColor>>) {

wathiede commented

2019-10-01 20:40:37 -07:00

Now that I look at the docs, sort_by() is defined on slice. So this function only needs colors to be sort_by()'able, why make it something that can go into vec?

Is it because the thing eventually needs to be a Vec, so make the parameter Into?

I get a little confused what the better parameter is smart containers or references to base types. It seems like &str is better than String, if your function allows it, so I was thinking &[TimeColor] would be better than Vec. Thoughts?

Now that I look at the docs, sort_by() is defined on slice. So this function only needs colors to be sort_by()'able, why make it something that can go into vec? Is it because the thing eventually needs to be a Vec, so make the parameter Into<Vec>? I get a little confused what the better parameter is smart containers or references to base types. It seems like &str is better than String, if your function allows it, so I was thinking &[TimeColor] would be better than Vec<TimeColor>. Thoughts?

ggriffiniii commented

2019-10-01 23:20:20 -07:00

Here are the benefits that I see, but I think there's plenty of room for reasonable people to disagree on these things.

Yes, it's because you eventually need a Vec to store in your struct. Since that's the end result there are potential efficiency gains by accepting anything that can be Into<Vec<TimeColor>> rather than &[TimeColor] and additional flexibility for the caller.

Consider the following ways to call the function if it accepts &[TimeColor]:

foo.set_colors(&[color1, color2]);
foo.set_colors(&vec![color1, color2]);

both of these will allocate a new vector within set_colors. The second case is really unfortunate because the caller just allocated their own vec just for the purpose of passing it to set_colors only to have set_colors allocate it's own identical copy.

Consider the following ways to call the function if it accepts impl Into<Vec<TimeColor>>:

foo.set_colors(&[color1, color2]);
foo.set_colors(&vec![color1, color2]);
foo.set_colors(vec![color1, color2]);

The first 2 are identical and result in identical code where set_colors allocates a new vector. The 3rd is the new variant. This will take the vec that's already created by the caller and pass it to set_colors and set_colors will simply sort it and assign it into the struct without needing to allocate a new vec.

Here are the benefits that I see, but I think there's plenty of room for reasonable people to disagree on these things. Yes, it's because you eventually need a `Vec` to store in your struct. Since that's the end result there are potential efficiency gains by accepting anything that can be `Into<Vec<TimeColor>>` rather than `&[TimeColor]` and additional flexibility for the caller. Consider the following ways to call the function if it accepts `&[TimeColor]`: ```rust foo.set_colors(&[color1, color2]); foo.set_colors(&vec![color1, color2]); ``` both of these will allocate a new vector within `set_colors`. The second case is really unfortunate because the caller just allocated their own vec just for the purpose of passing it to `set_colors` only to have `set_colors` allocate it's own identical copy. Consider the following ways to call the function if it accepts `impl Into<Vec<TimeColor>>`: ```rust foo.set_colors(&[color1, color2]); foo.set_colors(&vec![color1, color2]); foo.set_colors(vec![color1, color2]); ``` The first 2 are identical and result in identical code where `set_colors` allocates a new vector. The 3rd is the new variant. This will take the `vec` that's already created by the caller and pass it to `set_colors` and `set_colors` will simply sort it and assign it into the struct without needing to allocate a new `vec`.

wathiede closed this pull request

2019-10-03 11:19:52 -07:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: wathiede/i3xs#1