-
Notifications
You must be signed in to change notification settings - Fork 11
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
Teach IInstantJobRegistry to accept named jobs #215
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
/// <param name="jobName">The job name associated with this job.</param> | ||
/// <returns>Returns a <see cref="ParameterBuilder"/> that allows further configuration.</returns> | ||
/// <remarks>The job name should be unique over all job instances.</remarks> | ||
public ParameterBuilder WithName(string jobName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm now that isn't the best choice. The user can do the following:
j => j.WithName("Foo").WithName("Bar");
As the ParameterBuilder
also has a ParameterBuilder
.
To remove the ambiguity, we could have a WithParameter
property that directly returns the ParameterBuilder
:
j => j.WithParameter.WithName("Foo");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with a different proposal. Thoughts?
- Prevents the WithName().WithName() issue
- Allows the WithParameter().WithParameter() usage (for current fluent branches)
- Prevents the WithParameter().WithParameter() issue (for new fluent branch)
- Backward compatible (from an API standpoint)
May need to be cleaned up in v5 to come up with something tighter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start - but the API isn't quite there yet.
If the shape is what we want, we need some documentation for the feature and why it exists.
Doco and changelog are on my todo list once you like the API shape enough. |
f48b76f
to
b3459b8
Compare
We are 98% there. If we drop the abstract class, I like the API 100% ;) |
/// <summary> | ||
/// Chains another option to the job. | ||
/// </summary> | ||
#pragma warning disable CA1716 // Identifiers should not match keywords |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want, put this into the .editorconfig
It is not valid for the whole project, not just this place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
fe56469
to
1be45cf
Compare
@@ -60,7 +60,7 @@ public Task RunAsync(IJobExecutionContext context, CancellationToken token) | |||
var arguments = new object[parameters.Length]; | |||
for (var i = 0; i < parameters.Length; i++) | |||
{ | |||
if (parameters[i].ParameterType == typeof(JobExecutionContext)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As JobExecutionContext
isn't a public type, dynamic jobs can't even declare it as parameter.
Switching this to IJobExecutionContext
aligns with the what the doco advertizes
@linkdotnet Done |
@nulltoken Thanks again - for everything! |
Pull request description
Follow up to #213
Provides a design escape hatch trough new overloads in
IInstantJobRegistry
.PR meta checklist
main
branch for codeCode PR specific checklist