Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access fields in CrontabSchedule? #71

Closed
amferguson opened this issue Oct 2, 2020 · 8 comments
Closed

Access fields in CrontabSchedule? #71

amferguson opened this issue Oct 2, 2020 · 8 comments

Comments

@amferguson
Copy link

First, thanks so much for this library! It has been very useful. I apologize if this has been addressed already, but I couldn't find it in any past issues.

Do you have any plans (or objections?) to making it possible to get the fields along with their value range and their CrontabFieldKind from a CrontabSchedule? It looks like CrontabFieldImpl already has all of that exposed as public properties, but I don't see a way to actually access those from a schedule. Sorry if there's already a good way and I'm just missing it.

I was thinking maybe something like

var schedule = CrontabSchedule.Parse("00 05 17 * * 1-5");

var minutes = schedule.Fields.FirstOrDefault(f => f.Kind == CrontabFieldKind.Minute);

//Or maybe even 
minutes = schedule.Minutes;

Thanks!

Adam

@atifaziz
Copy link
Owner

Do you have any plans (or objections?)

There are no such plans as there's never been any such demand before (IIRC). It's hard to object without knowing for what purpose. You said it would give you access to more detailed information about individual fields but you didn't say much about the scenarios or use cases it would support. I honestly don't see a private implementation detail like CrontabFieldImpl becoming public; it's encapsulated to reserve the right to change. Exposing the field objects would also require committing to an expanded public API that I'm not sure is worth the trouble.

var minutes = schedule.Fields.FirstOrDefault(f => f.Kind == CrontabFieldKind.Minute);

The crontab fields are always in a very specific order. I don't see the value of dynamically identifying them. What is it that you will do with minutes after grabbing it?

@atifaziz
Copy link
Owner

Actually your request here has prompted me to create issue #72, which I consider to be a design bug. 😉

@MatisseHack
Copy link

For my case, I needed to know if some code was executing during a certain scheduled time period. I settled on something like this:

var now = DateTime.Now;
var expression = "* * * * *";
var parts = expression.Split(' ', StringSplitOptions.RemoveEmptyEntries);

if (parts.Length != 5)
    throw new Exception($"Cron expression '{expression}' does not have the correct number of parts (5)");

var match = CrontabField.Minutes(parts[0]).Contains(now.Minute)
    && CrontabField.Hours(parts[1]).Contains(now.Hour)
    && CrontabField.Days(parts[2]).Contains(now.Day)
    && CrontabField.Months(parts[3]).Contains(now.Month)
    && CrontabField.DaysOfWeek(parts[4]).Contains((int)now.DayOfWeek);

Exposing CrontabField properties on CrontabSchedule would be much preferred to this rudimentary parsing. I'm imagining something like this:

var now = DateTime.Now;
var schedule = CrontabSchedule.Parse("* * * * *");

var match = schedule.Minutes.Contains(now.Minute)
    && schedule.Hours.Contains(now.Hour)
    && schedule.Days.Contains(now.Day)
    && schedule.Months.Contains(now.Month)
    && schedule.DaysOfWeek.Contains((int)now.DayOfWeek);

@atifaziz
Copy link
Owner

atifaziz commented Oct 28, 2020

@MatisseHack Thanks for those examples. Always helps! Also, made me realise that #72 isn't a good idea for now so have abandoned it.

If I understand what you're trying to do, you want to poll whether the current time occurs in the schedule. Here's another way to accomplish the same without relying on CrontabField

Suppose you have an extension method that floors time to the minute component, dropping higher resolution components like seconds and milliseconds:

static partial class DateTimeExtensions
{
    public static DateTime FloorToMinute(this DateTime dt) =>
        new DateTime(dt.Year, dt.Month, dt.Day, dt.Hour, dt.Minute, 0);
}

Now you can write:

var now = DateTime.Now.FloorToMinute(); // current time to the minute
var schedule = CrontabSchedule.Parse("* * * * *");
var match = schedule.GetNextOccurrence(now.AddTicks(-1)) == now;

The now.AddTicks(-1) is needed to nudge it back slightly since GetNextOccurrence treats the input time as being exclusive.

What @amferguson is asking for here is for deeper access to internals like CrontabFieldImpl. I am still going to wait for him to come back on the use case for that.

@MatisseHack
Copy link

That is exactly what I wanted to accomplish!

I had considered a similar approach, but thought it might not be as clear what the intention of the code was. Now that I see your example, I think it is a reasonable alternative.

I would still love to have access to CrontabField properties on CrontabSchedule (perhaps a different issue than the original request), but your approach would unblock me if you decide to go ahead with #72.

Thanks for the advice!

@amferguson
Copy link
Author

Hi @atifaziz

Thanks for getting back to me! My use case is kind of specific, but not so dissimilar to @MatisseHack's. Basically, what I'm trying to do is convert a bunch of NCrontab schedules into Quartz's version of cron expressions on the fly. Because their formats don't line up exactly, I need to adjust the NCrontab ones when doing the conversion since Quartz is a bit finicky about the * character.

For example the NCrontab expression "15 00 * * *" becomes "00 15 00 * * ?" because it doesn't allow day-of-week and day-of-month to both be *. That is not the worst thing in the world, but it is a little bit gross looking to parse, split on the spaces, count the number of fields (since seconds are optional), etc.

Currently that looks (something) like

var fields = crontabSchedule.Split(' ').ToList();
if (fields.Count < 6)
{
  fields.Insert(0, "00");
}
if (fields[Days] == ........

Where things like Days are just constant int values relating to their position. The main reason I was asking if it were possible to get the fields by type (as NCrontab sees them) is to avoid some of that direct manipulation and to make the conversion a little smarter than just string manipulation. If it's not something you want to do, I've got a workable solution, but it just seemed a little redundant since NCrontab already knows which fields hold which value. Either way, thank you for your time and consideration! NCrontab is a great library that I have been very happy with for a number of years now.

@atifaziz
Copy link
Owner

atifaziz commented Nov 7, 2020

@amferguson Thanks for coming back with your scenario.

The main reason I was asking if it were possible to get the fields by type (as NCrontab sees them) is to avoid some of that direct manipulation and to make the conversion a little smarter than just string manipulation

I feel you should be able to get by reasonably well with what NCrontab exposes publicly today. I've put together a small demo (also posted as a gist you can download and explore) of how you could approach this:

#nullable enable

using System;
using System.Collections.Generic;
using System.Linq;
using NCrontab;

static class Program
{
    static readonly (CrontabFieldKind Kind, Func<string, CrontabField> Parser)[] CrontabFields =
    {
        (CrontabFieldKind.Second   , CrontabField.Seconds   ),
        (CrontabFieldKind.Minute   , CrontabField.Minutes   ),
        (CrontabFieldKind.Hour     , CrontabField.Hours     ),
        (CrontabFieldKind.Day      , CrontabField.Days      ),
        (CrontabFieldKind.Month    , CrontabField.Months    ),
        (CrontabFieldKind.DayOfWeek, CrontabField.DaysOfWeek),
    };

    static void Main(string[] args)
    {
        if (args.Length == 0)
            throw new Exception("Missing crontab expression argument.");

        var expressions = args[0].Split(' ', StringSplitOptions.RemoveEmptyEntries);
        if (expressions.Length < 5 || expressions.Length > 6)
            throw new Exception("Invalid crontab expression field count (must be 5 or 6).");

        foreach (var (expr, (kind, parser)) in
                 expressions.Zip(CrontabFields.TakeLast(expressions.Length)))
        {
            var field = parser(expr);
            Console.WriteLine($"{expr} : {kind} = {string.Join(", ", field.Values())}");
        }
    }

    static IEnumerable<int> Values(this ICrontabField field)
    {
        for (var n = field.GetFirst(); n >= 0; n = field.Next(n + 1))
            yield return n;
    }
}

Apart from the initial string split, you can parse each individual field expression into a CrontabField and then collect information from there.

Assuming you have downloaded the gist and your shell's current working directory is where you downloaded it, then you can run the above program for the expression 00 05 17 */2 * Mon-Fri using dotnet run as follows:

dotnet run -- '00 05 17 */2 * Mon-Fri'

You should then get the following output:

00 : Second = 0
05 : Minute = 5
17 : Hour = 17
*/2 : Day = 1, 3, 5, 7, 9, 11, 13, 15, 17, 19, 21, 23, 25, 27, 29, 31
* : Month = 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
Mon-Fri : DayOfWeek = 1, 2, 3, 4, 5

Hopefully, you can use this to do your conversion to Quartz's expressions. If it's a fair workaround then consider closing the issue.

@amferguson
Copy link
Author

Hi @atifaziz,
So sorry for the delay in getting back to you but thank you so much for the example! I think this will work very well, and I especially like the extension method you added. Thanks for taking the time to offer that up, and also again for the library itself. I'll go ahead and close this, because I don't think there's anything I won't be able to do with the API as-is.

Adam

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants