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

Add name parameter to spawn #385

Merged
merged 13 commits into from
Jan 27, 2023
Merged

Conversation

zoicsoftware
Copy link
Contributor

@zoicsoftware zoicsoftware commented Jan 16, 2023

What do these changes do?

These changes add the ability to optionally set the underlying Task name parameter on Jobs.

Are there changes in behavior for the user?

Setting the Task name during spawn

await scheduler.spawn(my_task(), name="my-task")
await scheduler.spawn(another_task(), name="another-task")
# name is optional, so the following still works just fine
await scheduler.spawn(another_task())

Getting All Job Task names in Scheduler

job_names = [job.name for job in scheduler]

Related issue number

Fixes #324

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

Copy link
Member

@Dreamsorcerer Dreamsorcerer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

tests/test_job.py Outdated Show resolved Hide resolved
tests/test_job.py Outdated Show resolved Hide resolved
aiojobs/_job.py Outdated Show resolved Hide resolved
aiojobs/_job.py Outdated Show resolved Hide resolved
@Dreamsorcerer
Copy link
Member

Also, tests failed on 3.7. So, we'll need an if sys.version_info > (3, 7): check.

@zoicsoftware
Copy link
Contributor Author

Thank you for the quick feedback!

With the desire for it to be get_name() to match the asycio syntax, do you think we should add a set_name() function to the Job so that the name API fully matches that of Task? It would also allow users to change the name after creation.

aiojobs/_job.py Outdated Show resolved Hide resolved
tests/test_job.py Outdated Show resolved Hide resolved
@Dreamsorcerer
Copy link
Member

With the desire for it to be get_name() to match the asycio syntax, do you think we should add a set_name() function to the Job so that the name API fully matches that of Task? It would also allow users to change the name after creation.

Yeah, could do.

aiojobs/_job.py Fixed Show fixed Hide fixed
Copy link
Member

@Dreamsorcerer Dreamsorcerer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only other thought is whether the user expects to get a (potentially different) name when the task doesn't exist. I'm wondering if it makes more sense to just error if the job hasn't been started yet. Not really settled on either side, just wondering if someone has any thoughts?

Otherwise, looking good, just need to resolve the mypy errors (can easily be seen locally by just running mypy as a command).

aiojobs/_job.py Outdated Show resolved Hide resolved
aiojobs/_job.py Outdated Show resolved Hide resolved
@zoicsoftware
Copy link
Contributor Author

My only other thought is whether the user expects to get a (potentially different) name when the task doesn't exist. I'm wondering if it makes more sense to just error if the job hasn't been started yet. Not really settled on either side, just wondering if someone has any thoughts?

I do agree it would be rather strange for the name to change without user action.

My opinion is that rather than throwing an exception we should do away with the "Job(coroutine)" default string and instead return None. So, a Job that has been given no explicit name and has not been started yet (or is python3.7) would simply return None. And that point, it truly has no name, so I think it make the most sense.

If we go down the route of "exception if Job not yet started", Is that an exception only if there is also not an explicit name given or always throw an exception if not started yet?

If its the later, I am not a big fan of having to worry about a Job that is potentially in a pending state throwing an exception. I think the largest use case for this feature is probably looping through a schedulers job set looking for a specific Job by a given name. I shouldn't have to worry about not getting the job if its still pending or possible exceptions while looking through the set and checking the name of each Job.

aiojobs/_job.py Fixed Show fixed Hide fixed
@Dreamsorcerer
Copy link
Member

My only other thought is whether the user expects to get a (potentially different) name when the task doesn't exist. I'm wondering if it makes more sense to just error if the job hasn't been started yet. Not really settled on either side, just wondering if someone has any thoughts?

My opinion is that rather than throwing an exception we should do away with the "Job(coroutine)" default string and instead return None. So, a Job that has been given no explicit name and has not been started yet (or is python3.7) would simply return None. And that point, it truly has no name, so I think it make the most sense.

Yep, OK, it makes sense that you'd want to cancel a job by name even if it's not started. Maybe going back to None makes sense then, where None means no explicit name given and job has not started yet.

aiojobs/_job.py Outdated Show resolved Hide resolved
aiojobs/_job.py Outdated Show resolved Hide resolved
aiojobs/_job.py Outdated Show resolved Hide resolved
aiojobs/_job.py Outdated Show resolved Hide resolved
tests/test_job.py Outdated Show resolved Hide resolved
tests/test_job.py Outdated Show resolved Hide resolved
@Dreamsorcerer Dreamsorcerer merged commit 935230e into aio-libs:master Jan 27, 2023
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

Successfully merging this pull request may close these issues.

Add name param to spawn
2 participants