[WIP] Make NCronJobOptionBuilder expose a fluent interface for untyped job

[nulltoken]

Pull request description

• Fix #218
• Fix #222

PR meta checklist

• [x] Pull request is targeted at main branch for code
• [x] Pull request is linked to all related issues, if any.

Code PR specific checklist

• [x] My code follows the code style of this project and AspNetCore coding guidelines.
• [x] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
• [x] My change requires a change to the CHANGELOG.
  • [ ] I have updated the appropriate sub section in the CHANGELOG.md.
• [x] I have added, updated or removed tests to according to my changes.
  • [x] All tests passed.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 93.13725% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...c/NCronJob/Configuration/Builder/JobRegistrator.cs	88.88%	4 Missing and 2 partials :warning:
src/NCronJob/NCronJobExtensions.cs	50.00%	1 Missing :warning:

Files with missing lines	Coverage Δ
...NCronJob/Configuration/Builder/JobOptionBuilder.cs	100.00% <100.00%> (ø)
...Job/Configuration/Builder/NCronJobOptionBuilder.cs	93.18% <100.00%> (+4.39%)	:arrow_up:
...CronJob/Configuration/Builder/RuntimeJobBuilder.cs	100.00% <100.00%> (ø)
src/NCronJob/Registry/JobRegistry.cs	89.28% <100.00%> (-0.10%)	:arrow_down:
src/NCronJob/Registry/RuntimeJobRegistry.cs	98.70% <100.00%> (+0.01%)	:arrow_up:
src/NCronJob/NCronJobExtensions.cs	95.65% <50.00%> (-2.13%)	:arrow_down:
...c/NCronJob/Configuration/Builder/JobRegistrator.cs	88.88% <88.88%> (ø)

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nulltoken]

@linkdotnet Although not 100% ready, I'd be a taker for a first pass of review please.

• JobRegistrator now holds the proposed V5 interface for registering jobs. The idea is to make NCronJobBuilder implement it (and delegate to JobRegistrator) during the move to v5.
• Untyped jobs and typed jobs are now registered in a similar way
• Untyped jobs can declare dependencies

[linkdotnet]

@nulltoken Good job!

I am fine as long as this version still works:

public NCronJobOptionBuilder AddJob(
        Delegate jobDelegate,
        string cronExpression,

I think the docs should showcase both and why both versions exist (simplicty over configurability and vice-versa). Currently, it only uses the above version.

[nulltoken]

@nulltoken Good job!

I am fine as long as this version still works:

public NCronJobOptionBuilder AddJob(
        Delegate jobDelegate,
        string cronExpression,

I think the docs should showcase both and why both versions exist (simplicty over configurability and vice-versa). Currently, it only uses the above version.

@linkdotnet I understand the need for simplicity.

However, both the docs and the README demonstrate Minimal API usage through something like

builder.Services.AddNCronJob((ILogger<Program> logger, TimeProvider timeProvider) =>
{
    logger.LogInformation("Hello World - The current date and time is {Time}", timeProvider.GetLocalNow());
}, "*/5 * * * * *");

How would you feel to keep allowing this inline usage through the AddNCronJob() IServiceCollection extension, but straighten the NCronJobBuilder to the proposed AddJob(Delegate, Action<JobOptionBuilder)?

Relevant:

• https://github.com/NCronJob-Dev/NCronJob?tab=readme-ov-file#minimal-job-api
• https://docs.ncronjob.dev/getting-started/#too-complicated
• https://docs.ncronjob.dev/features/minimal-api/

[linkdotnet]

That is fair if we keep both of them. I would be not onboard to drop the "string cron" version given that we want to give users the easiest setup possible to do something. Using an options builder already means the user has to know about some defaults (maybe).

Whether it is an extension or not is a different topic. I don't see the necessity here, given that everything is in one project anyway. It might make sense to "refactor" them out into a separate package.

That said, can you add some docs around the new builder. When to use (aka if you need more fine-grained control).

[nulltoken]

@linkdotnet I've dropped the deprecations in AddNCronJob() extension and NCronJobBuilder.AddJob(). I'll work on updating the doco in the following days.

[nulltoken]

@linkdotnet Can we at least deprecate

AddJob(
        Delegate jobDelegate,
        string cronExpression,
        TimeZoneInfo? timeZoneInfo = null,
        string? jobName = null)

and only keep the following one?

AddJob(
        Delegate jobDelegate,
        string cronExpression,
        TimeZoneInfo? timeZoneInfo = null)

I understand the need for simplicity.

But once one need a name for a job, I believe the simplicity is done and JobOptionBuilderoverload should be preferred.

Thoughts?

[linkdotnet]

@linkdotnet Can we at least deprecate

AddJob(
        Delegate jobDelegate,
        string cronExpression,
        TimeZoneInfo? timeZoneInfo = null,
        string? jobName = null)

and only keep the following one?

AddJob(
        Delegate jobDelegate,
        string cronExpression,
        TimeZoneInfo? timeZoneInfo = null)

I understand the need for simplicity.

But once one need a name for a job, I believe the simplicity is done and JobOptionBuilderoverload should be preferred.

Thoughts?

But the jobName is optional, isn't it? The idea behind the job name is solely that users have a way of disabling or re-enabling them later during runtime. Dees the string? jobName hinder you in any way?

Removing it would not only mean a major breaking change API-wise but also from a feature-set point of view.

[nulltoken]

@linkdotnet Can we at least deprecate

AddJob(
        Delegate jobDelegate,
        string cronExpression,
        TimeZoneInfo? timeZoneInfo = null,
        string? jobName = null)

and only keep the following one?

AddJob(
        Delegate jobDelegate,
        string cronExpression,
        TimeZoneInfo? timeZoneInfo = null)

I understand the need for simplicity. But once one need a name for a job, I believe the simplicity is done and JobOptionBuilderoverload should be preferred. Thoughts?

But the jobName is optional, isn't it?

Indeed, so for people not using it, there wouldn't be any change.

The idea behind the job name is solely that users have a way of disabling or re-enabling them later during runtime.

I understand that. And the RuntimeJobRegistry is certainly the most advanced feature in the library. This is why I was mentioning earlier that, given this intended usage, we may no longer be in "simplicity" land. This also explains why I'm advocating to redirect users requiring a jobName toward the AddJob() overload accepting a JobOptionBuilder.

Dees the string? jobName hinder you in any way?

Removing it would not only mean a major breaking change API-wise

Deprecating its usage for this overload would raise a warning directing the user to the recommended overload. It would indeed be removed in v5.

but also from a feature-set point of view.

Not really as the feature would still exist through the AddJob(Delegate, Action<JobOptionBuilder>) overload.

[nulltoken]

@linkdotnet Can we at least deprecate

AddJob(
        Delegate jobDelegate,
        string cronExpression,
        TimeZoneInfo? timeZoneInfo = null,
        string? jobName = null)

and only keep the following one?

AddJob(
        Delegate jobDelegate,
        string cronExpression,
        TimeZoneInfo? timeZoneInfo = null)

I understand the need for simplicity. But once one need a name for a job, I believe the simplicity is done and JobOptionBuilderoverload should be preferred. Thoughts?

But the jobName is optional, isn't it?

Indeed, so for people not using it, there wouldn't be any change.

The idea behind the job name is solely that users have a way of disabling or re-enabling them later during runtime.

I understand that. And the RuntimeJobRegistry is certainly the most advanced feature in the library. This is why I was mentioning earlier that, given this intended usage, we may no longer be in "simplicity" land. This also explains why I'm advocating to redirect users requiring a jobName toward the AddJob() overload accepting a JobOptionBuilder.

Dees the string? jobName hinder you in any way? Removing it would not only mean a major breaking change API-wise

Deprecating its usage for this overload would raise a warning directing the user to the recommended overload. It would indeed be removed in v5.

but also from a feature-set point of view.

Not really as the feature would still exist through the AddJob(Delegate, Action) overload.

@linkdotnet Have you had the time to gather your thoughts about this deprecation proposal? :wink:

[linkdotnet]

Hey hey @nulltoken - sure I did have some time but also totally forgot about that :D

I am still not a fan of forcing users to supply a jobName. The middle ground would be to automatically set the name (Guid.NewGuid or similiar friends).

If the user just needs a way of triggering a job on a frequent base but don't do anything else, why having a name?

That we need this internally, might make things easier and we can add some random string, I am fair with that, but don't see much value forcing the user.

But once one need a name for a job, I believe the simplicity is done and JobOptionBuilderoverload should be preferred.

That is, to some extent true - but more because of the way the library works. For example quartz returns an object (a trigger) which you can just "disable" easily. But fair for me, we can roll with your version and deprecate the current API, so the user has to use the builder.