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

propose to abandon @register_job #119

Closed
wants to merge 1 commit into from

Conversation

redstoneleo
Copy link
Contributor

@redstoneleo redstoneleo commented Oct 9, 2020

I think we should not recommend users registering jobs using django-apscheduler's @register_job decorator, since the id it automatically assigned to a job is a bit long and tedious, such as FinancialCharts.management.commands.runapscheduler.getIOER_CSV and
FinancialCharts.management.commands.runapscheduler.createLIBOR_Rate_csv, not that useful, I think we should encourage users to adopt APScheduler's decorator @scheduler.scheduled_job to register a job, which requires users manually name each id, so lead to better id readability . I just hold the Occam's razor principle here - "entities should not be multiplied without necessity", thus I think django-apscheduler's @register_job decorator should only reside in the source code for backward compatibility.

I think we should not recommend users registering jobs using django-apscheduler's  `@register_job` decorator, since the `id` it automatically assigned to a job is a bit long and tedious, such as `FinancialCharts.management.commands.runapscheduler.getIOER_CSV` and 
`FinancialCharts.management.commands.runapscheduler.createLIBOR_Rate_csv`, not that useful, I think we should encourage users to adopt APScheduler's decorator [`@scheduler.scheduled_job`](https://apscheduler.readthedocs.io/en/latest/modules/schedulers/base.html#apscheduler.schedulers.base.BaseScheduler.scheduled_job) to register a job, which require users manually name each `id`, so lead to better  `id` readability  . I just hold the  Occam's razor principle here -  "entities should not be multiplied without necessity", thus I think django-apscheduler's  `@register_job` decorator should only reside in the source code for backward compatibility.
@codecov-io
Copy link

codecov-io commented Oct 9, 2020

Codecov Report

Merging #119 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #119   +/-   ##
=======================================
  Coverage   97.80%   97.80%           
=======================================
  Files           6        6           
  Lines         319      319           
=======================================
  Hits          312      312           
  Misses          7        7           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd93595...293568a. Read the comment docs.

@jcass77
Copy link
Owner

jcass77 commented Oct 9, 2020

I share your dislike of @register_job and think that it can be deprecated altogether.

Closing this in favour of #120 that:

  • adds deprecation warning to @register_job
  • adds test case
  • adds changelog entry
  • removes mention of the decorated version of add_job: although it is still available it will not work with the example shown higher up in the readme. My experience is that this causes confusion for beginners, and seasoned APScheduler users won't need to be reminded of its existence anyway.

@jcass77 jcass77 closed this Oct 9, 2020
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.

None yet

3 participants