`every()` funcs final implementation

MahdiBM commented 3 years ago

Intro Notes

The same as the last PR, but just 1 commit instead of 31 😅. I thought this would be much cleaner to merge.
This is my first PR, so feel free to point out any issues, ever-so-obvious.
Also worth noting, I've tested this implementation with all three ways of unit-tests, manual-tests and in-production tests(!), and so far everything seems perfect.

What does this PR add to the queues package?

This PR adds the ability to schedule a ScheduledJob to be run at different times in any interval. For example you can call the .every(_:in:) functions like so .every(.seconds(5), in: .minutes(1)) and expect the schedule to be run every 5 seconds in a minute, indefinitely.

Motivation

I've not only seen other people be looking for such option in a long time, but i myself have been looking forward to having such options as well. I also saw the open issue here on github, and saw tanner suggesting the syntax .every for a function which allows people do the same thing that i implemented. That led me to think about contributing to the package by implementing this feature myself.

What exactly is implemented?

First of all, a generalized .every(_:in:underestimatedCount:) to be able to flexibly schedule a task to be done every while in an interval. This function is declared in the class ScheduleContainer.Builder, which is the equivalent to the ScheduleBuilder class prior to this PR. This every function is declared in ScheduleContainer as well for more convenience.
Second, convenience functions on top of Secondly, Minutely and Hourly to schedule a task every input amount of times in those types' respective intervals.

How was this implementations made possible?

When we want to schedule a job, we need to do like so: app.queues.schedule(ScheduledJob()) which prior to this PR, would return a class ScheduleBuilder. We then can add time specifications to the builder using the available functions like .at(Date()) or .minutely().at(5). The first problem in the way of implementing .every functions is that a builder is supposed to only contain one job and one time, and the ScheduleBuilder neither can contain more than one time for a job, nor can produce more schedules of its own kind which have different times than itself. To fix this, i came up with 2-3 different ways, and chose the one that you see. I decided to make a change as described below:

Make a ScheduleContainer so it can contain more than one builder.
return the ScheduleContainer instead of the builder when users do app.queues.schedule(_:)

What ScheduleContainer does is to hold multiple values of ScheduleContainer.Builder, plus the job that the builders are supposed to be scheduled with. So you'll see this in declaration of ScheduleContainer:

public let job: ScheduledJob
public var builders: [Builder] = []

public init(job: ScheduledJob) {
    self.job = job
}

I also changed Builder a little bit to hold a reference to its container, as well as using uuid to be able to identify builders that are in a container:

public let id = UUID()
public let container: ScheduleContainer

I also changed the way that a builder holds the time that it should be scheduled at, by introducing an enum:

public let id = UUID()
public let container: ScheduleContainer
private var _timeValue: TimeValue? = nil

TimeValue:

public enum TimeValue {
            case exact(date: Date)
            case intervalBased(offset: TimeInterval,
                               interval: TimeInterval,
                               isFirstLifecycle: Bool = true)
            case componentBased(month: Month? = nil,
                                day: Day? = nil,
                                weekday: Weekday? = nil,
                                time: Time? = nil,
                                minute: Minute? = nil,
                                second: Second? = nil,
                                nanosecond: Int? = nil)
}

Now a builder can clean-ly hold its time. This also helps when calculating the nextDate.

Another also, i declared convenience functions on top of ScheduleContainer which also avoids breaking people's codes. So as an example, the first function is in Builder, and the second one is in ScheduleContainer:

// MARK: - In `ScheduleContainer.Builder`:
/// Creates a monthly scheduled job for further building
public func monthly() -> Monthly {
    return Monthly(builder: self)
}

// MARK: - In `ScheduleContainer`
/// Creates a monthly scheduled job for further building
public func monthly() -> Builder.Monthly {
    return Builder.Monthly(builder: Builder(container: self))
}

One thing remaining, is that now that we have containers, one problem appears. The problem is that when we call app.queues.schedule(JOB()), the AnyScheduledJobs must not be immediately created. Instead, they should be created after the user is done with adding their builders to the container. If the app tries to create AnyScheduledJobs out of a container's job and builders immediately after the .schedule(_:) call happens, when it checks for builders, it'll see an empty array and basically no jobs will be added to the QueuesConfiguration. To tackle this i changed the scheduler to store all containers, and only calculate AnyScheduledJobs when needed:

// Previously:
var scheduledJobs: [AnyScheduledJob]

// Right now:
var scheduledJobsContainers: [ScheduleContainer]
var scheduledJobs: [AnyScheduledJob] {
    scheduledJobsContainers.map { container in
        container.builders.map { builder in
            AnyScheduledJob(job: container.job, scheduler: builder)
        }
    }.reduce(into: [AnyScheduledJob]()) { $0 += $1 }
}

This is the summary of what i've done. You can dig into the files more, and i've left some comments there as well. Feel free to ask any questions you might have, or point out any issues with this implementation.

My questions

These are basically the more-important things that i think i could improve:

In ScheduleContainer.swift line 338, Is underestimatedCount justified?
Currently the schedules built using .every func always start from the current time. I don't think thats an issue, but what are your opinions?
I also saw a @discardableResult behind the declarations of the .minutely function. I removed that since i couldn't figure out why it was there. Was that a mistake?
I think it is possible to release any containers that are completely done. Is that a good thing to implement?
Also feel free to suggest better names for the new stuff you see, if the names are not good enough.

0xTim commented 3 years ago

I haven't reviewed this yet, but deleting ScheduleBuilder, which is a public class is a non-starter unfortunately. That's a breaking API change we probably can't accept

MahdiBM commented 3 years ago

hmmm, i understand what you are talking about. BUT First: For clarity, this PR includes doesn't include real breaking changes syntax-wise. You can still do app.queues.shchedule(JOB()).at(Date()) and similar stuff, and those are still the right way to do it (Although app.queues.shchedule(JOB()) returns a ScheduleContainer).
Second: ScheduleBuilder is not really gone. It's now in the ScheduleContainer under the name Builder. I know by only changing the name; that's still a "breaking change" because the app won't compile, But my assumption is, and let me know if i'm wrong, that in reality no-one would really initialize a ScheduleBuilder themselves. They'd just let the app.queues.shchedule(_:) do it. Another assumption of mine is that notifying people that this may contain breaking changes will suffice.
so my questions are: 1- Mathematically yes, these are breaking changes, but are they really? I don't think so (I might be wrong; again, let me know) 2- Without such changes, i don't think it is possible to implement stuff like every() funcs without hacking around too much. Do the "breaking changes" matter enough that you guys prefer to let it be for another major version?

For the record i tried to not remove ScheduleBuilder but i found it very hacky to both not remove that class and implement the every funcs, so i gave up on that. That would have required a lot of inout parameters to pass around the QueuesConfiguration from what i understood, and thats not something i've seen in any projects. And even after that i wasn't sure it would work.

jdmcd commented 3 years ago

I agree with @0xTim - I haven't reviewed this yet but we definitely can't merge anything that would break the public API since we can't guarantee that no one is using it

MahdiBM commented 3 years ago

You both are not wrong, again. What i'm trying to say is that this is worth it although there is a 1/100 chance that this might be breaking.
I checked the Source code of the vapor/queues to see if there is any reason one would like to build a ScheduleBuilder themselves. I searched for ScheduleBuilder in Sources, and i found out it is being used in only 4 places: 1- In QueuesConfiguration there is a function which takes a ScheduleBuilder as one of its args. That function is internal. 2- AnyScheduledJob has a var named scheduler of type ScheduleBuilder. AnyScheduledJob is internal. 3- Inside ScheduleBuilder.Yearly, ScheduleBuilder.Minutely and ... . Again, all internal. 4- queues.schedule(_:) this one is the only place in the whole Queues package that a ScheduleBuilder goes outside.

So here are possibilities i can think of that one would suffer from ScheduleBuilder being removed:
1- If someone for some unknown reasons that i don't think neither of us 3 can imagine, has decided to actually init a ScheduleBuilder himself. That would be for use outside the Queues package because you can't possibly push a ScheduleBuilder in queues. 2- If someone has decided to do something like this and explicitly declare the type as ScheduleBuilder:

// Explicit `: ScheduleBuilder`?!
let builder: ScheduleBuilder = app.queues.schedule(JOB())
builder.minutely().at(5)

They'll need to replace ScheduleBuilder with ScheduleContainer, which is a trivial task. 3- This one is the most legitimate reason i could find. If someone is using some of the types inside ScheduleBuilder someway that they needed to declare the root type. something like:

let minute: ScheduleBuilder.Minute = 5

They'll need to replace ScheduleBuilder with either ScheduleContainer or ScheduleContainer.Builder, which is a trivial task.
To fix both above issues, i'm thinking i can move all sub-structs to the ScheduleContainer (like they were before this PR, in ScheduleBuilder), then add a typealias public typealias ScheduleBuilder = ScheduleContainer and put a deprecation notice on top of it. I can make this happen if you guys think it is needed.

The question that remains is that do you guys think those are enough reasons to delay these changes to a major version?
Having tried a lot; i don't think it is possible to implement what i've done, in a good fashion, without changes less than this. I was mindful of not-breaking people's code and this was the best i could pull-off.

All those being said, the way the every() func works has 2 benefits as well: 1- If you use every() func, the issues #94 and #85 won't be produced. 2- This PR also clears the way for a lot of other useful functions that need some kind of ScheduleContainer to be able to replicate themselves and turn into multiple schedules instead of one. I'm planning to add 2-3 of them which will basically make the Queues scheduling as powerful as it can get. One of those funcs i can remember on top of my head is a times() func to do a job the input amount of times in an interval.

I respect you two's decision if you think this PR is still not appropriate for a minor-version. I'll wait for you to review the code and notify me of your decision; or ask any questions if you have.

0xTim commented 3 years ago

Unfortunately breaking the public API under any circumstance is not possible for a non-major release. Getting build errors when running package updates destroys trust in the ecosystem so we can't allow it.

However, if there is no reasonable way to get this in with a non-breaking change we could release a major version with it in. There's nothing saying we can't release a major version update tomorrow

MahdiBM commented 3 years ago

So lesson 1 learned ✅: No breaking API changes in minor versions!

About releasing a major version, i think we'll need to know what @jdmcd thinks. I personally have no problem with that, and think this is a good point to start improving the package both because its been stale for a while now and because i believe something like this PR does remove some obstacles which are in the way of this package getting better.

MahdiBM commented 1 year ago

This probably shouldn't stay opened. It's breaking and if we were to publish a new major version, we can do better than this.

vapor / queues