Option in TimeUnitField using lazy_static #76

koenichiwa · 2021-02-17T14:53:44Z

Same as #73 Just cleaned up the git mess.

Changes the internals of all TimeUnitFields to be optional. If the TimeUnitField specified, then its populated with an OrdinalSet. If it's not specified (the cron field is *) then it's None. If then the the OrdinalSet is requested, it will simply return a reference to a static OrdinalSet that is filled using [TimeUnitField]::supported_ordinals(). TimeUnitField::all() will create a TimeUnitField that will point to this static set.

TimeUnitSpec gains an extra function: is_specified()

I've chosen to populate this set lazily, but I had to make it statically, because the following code simply doesn't work:

fn ordinals(&self) -> &OrdinalSet {
        &self.0
        match &self.ordinals {
            Some(ordinal_set) => &ordinal_set,
            None => &self::all() // Can't return this reference, needs to be a static reference.
        }
    }

Lastly I cleaned up some small stuff in parsing.rs

Seconds::from_ordinal_set(iter::once(0).collect()) becomes Seconds::from_ordinal(0) (etc.)

Also fix cherry-picking mistake ( see parsing.rs from_ordinal(_set)

koenichiwa · 2021-02-17T15:02:29Z

Decided to write some tests, found a small mistake.

Apparently is_specified was only in TimeUnitField.

zslayton

Why is lazy_static preferable to something like OnceCell? I don't have much experience with either; if there's no great answer I'm happy to move ahead with lazy_static.

cron has a lot of formatting foibles. When we reach a stopping place in the round of PRs you're working on I'd like to do a pass with rustfmt and add that to the CI build.

zslayton · 2021-02-17T15:16:21Z

src/time_unit/days_of_week.rs


 impl TimeUnitField for DaysOfWeek {
-    fn from_ordinal_set(ordinal_set: OrdinalSet) -> Self {
-        DaysOfWeek(ordinal_set)
+    fn from_ordinal_set(ordinal_set: Option<OrdinalSet>) -> Self {


Accepting an Option<_> works fine, but to my eyes DaysOfWeek::from_ordinal_set(None) looks like it's using the empty set rather than all values.

Another option here: the TimeUnitField trait could have three methods, a low-level one that accepts an Option<OrdinalSet> (essentially a rename of the existing from_ordinal_set) and two default impls that expose a more self-documenting API for use elsewhere in the crate like parsing.rs:

// Each type implements this: fn from_optional_ordinal_set(ordinal_set: Option<OrdinalSet>) -> Self; // === The trait provides these fn impls for free === fn all() -> Self { Self::from_optional_ordinal_set(None) } fn from_ordinal_set(ordinal_set: OrdinalSet) -> Self { Self::from_optional_ordinal_set(Some(ordinal_set)) }

Yeah no problem! I'll do that.

zslayton · 2021-02-17T15:21:33Z

src/time_unit/mod.rs

@@ -236,7 +237,7 @@ where
        use self::Specifier::*;
        //println!("ordinals_from_specifier for {} => {:?}", Self::name(), specifier);
        match *specifier {
-            All => Ok(Self::supported_ordinals()),
+            All => Ok(Self::supported_ordinals()),  // TODO: change to Self::ALL


Can you tell me about this TODO? Off-hand it seems like Self::supported_ordinals() and Self::ALL.clone() would do the same work. (The latter might even do more work because lazy_static has a small amount of overhead to dereference.)

Self::supported ordinals creates a new OrdinalSet. If ALL is already initialized, this should run quicker, otherwise ALL initializes the OrdinalSet for the next use (instead of throwing the OrdinalSet away)

ALL is a static ref though, and would need to be clone()ed to satisfy the function's return type.

That would defeat the purpose then. I can remove the TODO

koenichiwa · 2021-02-17T15:30:46Z

Why is lazy_static preferable to something like OnceCell? I don't have much experience with either; if there's no great answer I'm happy to move ahead with lazy_static.

Never heard of OnceCell, but this is the first sentence I read on the link you send:

This is essentially the lazy_static! macro, but without a macro.

I can try to change it to OnceCell if you prefer.

from_ordinal_set takes an OrdinalSet again, and is implemented in TimeUnitField

koenichiwa · 2021-02-17T15:55:54Z

I'm looking more into OnceCell. It seems that there is a proposition to move it into std. Maybe it actually is better to use that instead of lazy_static.

src/parsing.rs

zslayton · 2021-02-17T16:02:27Z

src/time_unit/mod.rs

+    /// assert_eq!(true, schedule.days_of_month().is_specified());
+    /// assert_eq!(false, schedule.months().is_specified());
+    /// ```
+    fn is_specified(&self) -> bool;


I have a minor quibble with the name of this function: the field is specified; the user had to write *. How would you feel about is_all instead, which would return the inverse?

This is slightly different in that (for example) a DayOfWeek field with *, ?, and 1-7 would all return true for is_all() regardless of whether its internal representations were None or the equivalent of Some(ALL).

I like the feel of this better; would it address your use case?

I'm a bit afraid that this implementation would break when you get to years though. If I would type Years.is_all() on 1970-2070 it should actually always return false right? Or maybe the end is actually nigh :p

Yes you can specify 1-7 for weekdays, but then you actually did specify it, as being all weekdays.

My use case was (basically the reason why I came about this project) to change cron strings into launchd files. When you don't specify a year in a launchd file, it means that you want something to happen regardless of the year. I didn't want to print 100 or so years in my launchd file whenever I parsed a string with a * in it.

I'm a bit afraid that this implementation would break when you get to years though. If I would type Years.is_all() on 1970-2070 it should actually always return false right? Or maybe the end is actually nigh :p

Ha, good point. The impl I laid out would be the default impl, but I think we could easily justify overriding it in the impl for Years to just look at is_some().

Yes you can specify 1-7 for weekdays, but then you actually did specify it, as being all weekdays.

I see the distinction you're making, but I'm not certain I want Schedule to make that distinction. There's not currently a distinction between * and ? (for fields that accept it), and intuitively I would expect 1-7 (or whichever inclusive range) to be treated the same for fields that have a fixed range (all of them but Years).

My use case was (basically the reason why I came about this project) to change cron strings into launchd files. When you don't specify a year in a launchd file, it means that you want something to happen regardless of the year. I didn't want to print 100 or so years in my launchd file whenever I parsed a string with a * in it.

Does the default impl override of is_all for Years address this for you?

To be clear, I could be persuaded to have an is_specified method, it just feels a bit hazy semantically. I wanted to see if we could get by with something that wasn't strictly bound to the input expression syntax first.

zslayton · 2021-02-17T16:09:11Z

src/time_unit/minutes.rs

+        }
+    }
+    fn is_specified(&self) -> bool {
+        self.ordinals.is_some()


If someone wrote 0-23 for the hours field, that would be equivalent to * but this would return false. Is that the intended effect? What if this were something like:

Suggested change

self.ordinals.is_some()

let max_supported_ordinals = (Self::inclusive_max() - Self::inclusive_min()) + 1;

let is_all = self.ordinals

.map(|o| o.len() == max_supported_ordinals)

.unwrap_or(true);

!is_all

This could be a default impl shared across all TimeUnitField types.

(If this were renamed from is_specified to is_all we'd obviously drop the ! in the returned value.)

Thios could be an option. I feel like is_all is still a really weird function to call on years though

If I'd make that change, I would rather write it as:

fn is_all(&self) -> bool { let max_supported_ordinals = Self::inclusive_max() - Self::inclusive_min() + 1 self.ordinals.map_or(true, |o| o.len() == max_supported_ordinals)) }

Yep, that's better. 👍 Didn't know about map_or.

Didn't know about map_or.
Me neither till I tried running rustfix --pedantic. Might not be a bad idea to run that after this PR cycle.

I can do this, but ordinals() in TimeUnitField doesn't return an Option so I just changed it to

fn is_all(&self) -> bool { let max_supported_ordinals = Self::inclusive_max() - Self::inclusive_min() + 1; self.ordinals().len() == max_supported_ordinals as usize }

Which I'm sure is acceptable as well

zslayton · 2021-02-17T16:10:52Z

src/time_unit/mod.rs

@@ -236,7 +237,7 @@ where
        use self::Specifier::*;
        //println!("ordinals_from_specifier for {} => {:?}", Self::name(), specifier);
        match *specifier {
-            All => Ok(Self::supported_ordinals()),
+            All => Ok(Self::supported_ordinals()),  // TODO: change to Self::ALL


ALL is a static ref though, and would need to be clone()ed to satisfy the function's return type.

koenichiwa · 2021-02-17T16:51:10Z

Should I use lazy_static or once_cell?
To the first approximation, once_cell is both more flexible and more convenient than lazy_static and should be preferred.
Unlike once_cell, lazy_static supports spinlock-based implementation of blocking which works with #![no_std].
lazy_static has received significantly more real world testing, but once_cell is also a widely used crate

From the once_cell docs. I can change it from lazy_static without any problems. Yea or nay?

zslayton · 2021-02-17T17:27:47Z

Should I use lazy_static or once_cell?
To the first approximation, once_cell is both more flexible and more convenient than lazy_static and should be preferred.
Unlike once_cell, lazy_static supports spinlock-based implementation of blocking which works with #![no_std].
lazy_static has received significantly more real world testing, but once_cell is also a widely used crate

From the once_cell docs. I can change it from lazy_static without any problems. Yea or nay?

Yea. Thanks for looking into this!

Also remove TODO from mod.rs

koenichiwa · 2021-02-17T18:41:40Z

Ok those were a couple of extra pushes, but I think I'm done now. I changed lazy_static to once_cell. I changed is_specified to is_all. Then realized it was implemented in the wrong position, so I had to change that. And lastly I changed the test string to be "0-59 * 0-23 ?/2 1,2-4 ? *". I think that covers everything.

zslayton

Were you going to add an overriding impl of is_all for Years to check is_some? Otherwise this looks great.

src/time_unit/mod.rs

koenichiwa · 2021-02-18T06:42:29Z

Were you going to add an overriding impl of is_all for Years to check is_some? Otherwise this looks great.

I had a bit of a problem with the name is_all in relation to years. That's why I preferred is_specified. But I do need to check in some way if * is used for years. At the moment it checks if years contains all the years that this library can handle. If it is decided that the bounds of years are changed, this check will still work.
So I'd rather leave it like this

Nit on line #160 Co-authored-by: Zack Slayton <zack.slayton@gmail.com>

zslayton · 2021-02-18T13:26:30Z

I had a bit of a problem with the name is_all in relation to years. That's why I preferred is_specified. But I do need to check in some way if * is used for years.

My objection to is_specified was/is largely a naming problem. To me, the only way for a value to be unspecified is for that field to be absent, which is currently only legal for Years (though I'd like it to be possible for Seconds too, #13).

Now that we have an is_all method that treats ?, *, (e.g.) 1-7 and <absent> as equivalent, I think I would be ok having a separate is_specified method that is false for <absent> and true for any other expression.

Would that combination of is_specified and is_all satisfy your use case? Or do you need to be able to further distinguish between 1970-2070 and *, which I think is the only "all" equivalence possible in the Years field?

If it is decided that the bounds of years are changed, this check will still work. So I'd rather leave it like this.

Not sure I followed this. Could you give an example?

zslayton · 2021-02-18T13:28:03Z

I'm going to go ahead and merge this PR, we can resolve the outstanding discussion points in a follow-on.

Thanks again!

koenichiwa · 2021-02-18T13:37:01Z

My objection to is_specified was/is largely a naming problem. To me, the only for a value to be unspecified is for that field to be absent, which is currently only legal for Years (though I'd like it to be possible for Seconds too, #13).

Yeah my problem with is_all was largely a naming problem as well. I but I don't thionk we have to agree on every nitpick. It's your project after all.

Not sure I followed this. Could you give an example?

The current bounds are 1970-2070, if someone comes along and decides that these bounds are insufficient. Then the is_all function on years will still work. of course it's not literaly all years, but yeah... nitpick

koenichiwa added 3 commits February 17, 2021 15:34

Option in TimeUnitField using lazy_static

1457710

Add Option<_> to constructor

284c2f6

Fix lazy_static value. Use None RootSpecifier is Specifier::All

a8226dc

Also fix cherry-picking mistake ( see parsing.rs from_ordinal(_set)

koenichiwa mentioned this pull request Feb 17, 2021

Feature/fields as options #73

Closed

Add is_specified to TimeUnitSpec, with comments. Add test

3edc5ce

Apparently is_specified was only in TimeUnitField.

zslayton reviewed Feb 17, 2021

View reviewed changes

Add from_optional_ordinal_set

8e6249c

from_ordinal_set takes an OrdinalSet again, and is implemented in TimeUnitField

zslayton reviewed Feb 17, 2021

View reviewed changes

koenichiwa added 5 commits February 17, 2021 18:31

Change lazy_static to once_cell.

f820cd3

Also remove TODO from mod.rs

Change is_specified to is_all

16e200c

Change test to check more cases

b700aad

Change test to check *even* more cases

e785cd5

Change position of is_all

1c652d2

zslayton reviewed Feb 17, 2021

View reviewed changes

src/time_unit/mod.rs Outdated Show resolved Hide resolved

Update src/time_unit/mod.rs

9157d47

Nit on line #160 Co-authored-by: Zack Slayton <zack.slayton@gmail.com>

zslayton merged commit 22ea6bc into zslayton:master Feb 18, 2021

koenichiwa deleted the feature/optional-fields branch February 23, 2021 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option in TimeUnitField using lazy_static #76

Option in TimeUnitField using lazy_static #76

koenichiwa commented Feb 17, 2021 •

edited

Loading

koenichiwa commented Feb 17, 2021

zslayton left a comment

zslayton Feb 17, 2021

koenichiwa Feb 17, 2021

zslayton Feb 17, 2021

koenichiwa Feb 17, 2021 •

edited

Loading

zslayton Feb 17, 2021

koenichiwa Feb 17, 2021

koenichiwa commented Feb 17, 2021

koenichiwa commented Feb 17, 2021 •

edited

Loading

zslayton Feb 17, 2021 •

edited

Loading

koenichiwa Feb 17, 2021

zslayton Feb 17, 2021 •

edited

Loading

zslayton Feb 17, 2021

zslayton Feb 17, 2021

koenichiwa Feb 17, 2021

koenichiwa Feb 17, 2021

zslayton Feb 17, 2021

koenichiwa Feb 17, 2021

zslayton Feb 17, 2021

koenichiwa commented Feb 17, 2021

zslayton commented Feb 17, 2021

koenichiwa commented Feb 17, 2021

zslayton left a comment

koenichiwa commented Feb 18, 2021 •

edited

Loading

zslayton commented Feb 18, 2021 •

edited

Loading

zslayton commented Feb 18, 2021

koenichiwa commented Feb 18, 2021

-        self.ordinals.is_some()
+        let max_supported_ordinals = (Self::inclusive_max() - Self::inclusive_min()) + 1;
+        let is_all = self.ordinals
+            .map(|o| o.len() == max_supported_ordinals)
+            .unwrap_or(true);
+        !is_all

Option in TimeUnitField using lazy_static #76

Option in TimeUnitField using lazy_static #76

Conversation

koenichiwa commented Feb 17, 2021 • edited Loading

koenichiwa commented Feb 17, 2021

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koenichiwa Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koenichiwa commented Feb 17, 2021

koenichiwa commented Feb 17, 2021 • edited Loading

zslayton Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zslayton Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koenichiwa commented Feb 17, 2021

zslayton commented Feb 17, 2021

koenichiwa commented Feb 17, 2021

zslayton left a comment

Choose a reason for hiding this comment

koenichiwa commented Feb 18, 2021 • edited Loading

zslayton commented Feb 18, 2021 • edited Loading

zslayton commented Feb 18, 2021

koenichiwa commented Feb 18, 2021

koenichiwa commented Feb 17, 2021 •

edited

Loading

koenichiwa Feb 17, 2021 •

edited

Loading

koenichiwa commented Feb 17, 2021 •

edited

Loading

zslayton Feb 17, 2021 •

edited

Loading

zslayton Feb 17, 2021 •

edited

Loading

koenichiwa commented Feb 18, 2021 •

edited

Loading

zslayton commented Feb 18, 2021 •

edited

Loading