Make it ready for v0.9.0

[koenichiwa]

Because I fixed two of the metaissues in #67 and I would like to use them easily in my own personal project. I was wondering what could be done to make this project ready for v0.9.0

I was thinking a couple of things:

• Run a more heftier version of clippy against the codebase.
  • I tried running clippy::pedantic and some parts of clippy::nursery, and even though it did have a couple of false positives it did help me spot a couple of issues. E.g. unneeded borrows, changing .map(f).unwrap_or_else(g) into .map_or_else(g,f), or with ranges: (min..max + 1) into (min..=max)
• Small refactor
  • Giving Field it's own file and ScheduleFields it's own file. Also changing the location of things like FromString for Schedule to the schedule.rs file.

If there are any other issues that need to be addressed I would love to see it.

Edit:

• Relicense to dual MIT/Apache-2

[zslayton]

These are both good changes to make. One other I'd like to squeeze into v0.9.0 is a fix for #8, changing the license from MIT to dual MIT/Apache-2. The changes you've made since v0.8.0 are substantial though, so I'd be ok doing a release without that change if you'd prefer.

[koenichiwa]

Yeah sure! I'm not really at home with licensing though, so if you can do that, then I can work on the other two issues?

[zslayton]

I'm not really at home with licensing though

Me neither. :) I can take care of it, sure.

[koenichiwa]

Quick question about OrdinalIter OrdinalRangeIter etc. Shouldn't they actually reside in the ordinal module?

Edit:
I was also looking into making them just type definitions like so:

pub type Ordinal = u32;
pub type OrdinalSet = BTreeSet<Ordinal>;
pub type OrdinalIter<'a> = btree_set::Iter<'a, Ordinal>;
pub type OrdinalRangeIter<'a> = btree_set::Range<'a, Ordinal>;

Which could've been so clean
But I was getting this error

error[E0308]: mismatched types
   --> tests/lib.rs:294:9
    |
294 |         assert_eq!(Some(15), paydays.next());
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected integer, found `&u32`
    |
    = note: expected enum `Option<{integer}>`
               found enum `Option<&u32>`
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

So my guess is that the iterator of BTreeSet returns &&u32? Any idea why this happens?

Edit on edit:
Maybe this is why my implementation of the queries was so slow? Can it be because the current implementation of iter copies every Ordinal before returning it? Isn't this exactly a reason why this should be implemented as a type definition like above, so the end user can chose whether to copy or not?

[zslayton]

Quick question about OrdinalIter OrdinalRangeIter etc. Shouldn't they actually reside in the ordinal module?

Sounds sensible to me.

Maybe this is why my implementation of the queries was so slow? Can it be because the current implementation of iter copies every Ordinal before returning it?

It's impossible to rule it out without testing/measurement, but that would surprise me. An Ordinal is a u32, so it fits in a register on x86_64 machines. Copying it should be trivial.

Isn't this exactly a reason why this should be implemented as a type definition like above, so the end user can chose whether to copy or not?

The copying that's happening is basically a dereference of the &u32 provided by the BTreeSet iterator. If we returned the &u32 directly to the user, they'd just end up doing the dereference themselves.

I'm happy to be proven wrong if you'd like to dig into it!

[koenichiwa]

Welp, I guess your right. The execution time changed from 186 to 184, which is insignificant, and it was a pain to code with.
I have a counter offer though, what if it gets implemented as

pub type OrdinalIter<'a> = Cloned<btree_set::Iter<'a, Ordinal>>;
pub type OrdinalRangeIter<'a> = Cloned<btree_set::Range<'a, Ordinal>>;

Then in the time_unit mod:

impl<T> TimeUnitSpec for T
where
    T: TimeUnitField,
{
//...
 fn iter(&self) -> OrdinalIter {
        TimeUnitField::ordinals(self).iter().cloned()
    }
//...
}

That way it looks way cleaner, and as a bonus you get all btree_set::Iters traits for free whithout anything changing.

[zslayton]

pub type OrdinalIter<'a> = Cloned<btree_set::Iter<'a, Ordinal>>;
pub type OrdinalRangeIter<'a> = Cloned<btree_set::Range<'a, Ordinal>>;

Based on a hazy recollection of implementing this years ago, I believe my main concerns with using a type alias were that:

• It leaks implementation details to users of the crate. At least at the time, error messages would contain the aliased type names (Cloned<btree_set::Iter<'a, Ordinal>>), making it clear how internals were being implemented and effectively making those types part of the public API.
• It prevented me from having control over what OrdinalIter could and couldn't do. If btree_set::Iter added or removed functionality, cron's API would change whether I liked it or not.

Combined with the fact that the compiler will reduce the layout of a struct with a single field into that field's layout, it seemed reasonable to just make my own wrapper type.

I wish Rust offered a version of newtype that was syntactically closer to a type alias. 😞

as a bonus you get all btree_set::Iters traits for free whithout anything changing

That's compelling! Unfortunately, I think the public API exposure is enough of a problem that it's worth exposing those individually/manually. Which traits have you found yourself missing?

[koenichiwa]

It leaks implementation details to users of the crate. At least at the time, error messages would contain the aliased type names (Cloned<btree_set::Iter<'a, Ordinal>>), making it clear how internals were being implemented and effectively making those types part of the public API.

I honestly didn't know that. But I get that that is a concern.

It prevented me from having control over what OrdinalIter could and couldn't do. If btree_set::Iter added or removed functionality, cron's API would change whether I liked it or not.

I doubt that those kind of breaking changes to the standard library would happen without an opt-in. Maybe if you change the rust edition. Otherwise a whole boatload of rust libraries would have the same problem.
One of the first lines of the std::collections module README states:

By using the standard implementations, it should be possible for two libraries to communicate without significant data conversion.

So I thought I'd live by that "credo" as much as possible

That's compelling! Unfortunately, I think the public API exposure is enough of a problem that it's worth exposing those individually/manually. Which traits have you found yourself missing?

I'm not missing anything in particular. I was just trying to make everything as neat as possible, so that you can reason about the project a bit more easily. I just thought that the implementation of a Range and an Iter was outside of the scope of the project.

If I'm going to refactor it without a typealias, then I just need to either make the field pub, or create a pub constructor. The module is private though, so it will only be exposed to the crate.
Another Implementation could be to keep the typealiases, make the ordinal module public (with the proper documentation), and tell people to RTFM. The module is now private, and because of that an Ordinal in TimeUnitSpec.includes is currently exposed as a u32. IMHO I would rather go with this implementation, but it's your call.

Edit:
If the module is public, the docs will change the variable type in TimeUnitSpec.includes from u32 to Ordinal. I might take a deeper dive to see if that's the reason why the error messages state the wrong type.

Edit on edit:

let foo: i32 = schedule.years().iter();

Still gives me

error[E0308]: mismatched types
  --> src/main.rs:12:20
   |
12 |     let foo: i32 = schedule.years().iter();
   |              ---   ^^^^^^^^^^^^^^^^^^^^^^^ expected `i32`, found struct `Cloned`
   |              |
   |              expected due to this
   |
   = note: expected type `i32`
            found struct `Cloned<std::collections::btree_set::Iter<'_, u32>>`

Even though I made the typealias public. So I get your concern.

[koenichiwa]

On an unrelated note, I have two nits.

• OrdinalRangeIter does not implement Iter, shouldn't it get renamed to OrdinalRange
• ScheduleIterator is returned by some public functions of Schedule. But because the type itself is not public, the end user can't read up on it in the docs. Shouldn't it be public (with a private constructor of course), just so users know what they're working with?

Sorry if coming of as too direct or too critical about a project that isn't mine, I'm just trying to help.

[zslayton]

It's likely to be a day or two before I have a chance to go through the changes you're describing.

Sorry if coming of as too direct or too critical about a project that isn't mine, I'm just trying to help.

Not at all, I appreciate it. It's great to have a second opinion on these things, especially if the library ever hopes to release a v1.0!

[zslayton]

If I'm going to refactor it without a typealias, then I just need to either make the field pub, or create a pub constructor.

I think I've missed a beat: what's the use case for users creating their own ordinal iterators?

OrdinalRangeIter does not implement Iter, shouldn't it get renamed to OrdinalRange

I'm not aware of a trait called Iter. If you're referring to Iterator, though, it does implement that:

cron/src/time_unit/mod.rs

Lines 42 to 51 in 2bbb4f2

	pub struct OrdinalRangeIter<'a> {
	range_iter: btree_set::Range<'a, Ordinal>,
	}
	impl<'a> Iterator for OrdinalRangeIter<'a> {
	type Item = Ordinal;
	fn next(&mut self) -> Option<Ordinal> {
	self.range_iter.next().copied()
	}
	}

(Sorry if there's actually an Iter trait--could you point me to it?)

ScheduleIterator is returned by some public functions of Schedule. But because the type itself is not public, the end user can't read up on it in the docs. Shouldn't it be public (with a private constructor of course), just so users know what they're working with?

Huh! While the type itself is public:

cron/src/schedule.rs

Lines 342 to 349 in d0049fc

	pub struct ScheduleIterator<'a, Z>
	where
	Z: TimeZone,
	{
	is_done: bool,
	schedule: &'a Schedule,
	previous_datetime: DateTime<Z>,
	}

(which allows it to be returned by public fns in Schedule), the schedule module it lives in is not, which prevents it from appearing in documentation. Interesting. I think the simplest fix is to re-export ScheduleIterator in lib.rs like we do for Schedule and TimeUnitSpec:

cron/src/lib.rs

Lines 50 to 51 in d0049fc

	pub use crate::schedule::Schedule;
	pub use crate::time_unit::TimeUnitSpec;

Good eye!

[koenichiwa]

think I've missed a beat: what's the use case for users creating their own ordinal iterators?

Oh no, I meant exactly the opposite! If I refactor it out to the ordinal mod, it needs a crate public function so it can be used in the time_unit mod. But it should stay private for the user.

I'm not aware of a trait called Iter. If you're referring to Iterator, though, it does implement that:

Oops, it seems I've made a mistake. I was lookming at the Range and Iter structs in BTreeSet. As far as I can see, range isn't even an existing trait. But it still might be an idea to follow convention and call it OrdinalRange.

Huh! While the type itself is public

Yes, I think that's because the module is private. I think as the code after the rfactor stands right now, everything in that module can be public. (I would still do that using re-exports though, otherwise you'd need to type use cron::schedule::Schedule)
Concerning the exports, the same goes for Ordinal, OrdinalIter and OrdinalRangeIter. They're returned, but their documentation should be public as well

[zslayton]

Sorry, I let the ball drop on this thread. 😞 I'll try to revisit it later this week.

For the moment: I've released v0.9.0 to make the fix from #84 available. I'll do another release once this change set lands. You dominated the release notes for this one, thanks again for all your help!

[koenichiwa]

Nothing to worry about. My study picked up the pace again, so I'm focusing on other stuff currently. We'll get back to this later 🙂

[koenichiwa]

Having said that. I was bored, and thought I would implement ordinal.rs back to the struct/impl implementation. I left the type aliases commented though, so you can make a comparison, tell me which direction we should take, and what should be deleted