-
Notifications
You must be signed in to change notification settings - Fork 4
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
Adding celery scaffold to the project. #36
Conversation
51c1ca1
to
bf1c5f3
Compare
The changes to tox and the GitHub action are due to an issue in import lib-metadata in python 3.7 see python/importlib_metadata#411 for more details. If we no longer need 3.7 support we could just remove that from tox and the action and everything should just work. |
bf1c5f3
to
d7e752a
Compare
I would advocate removing 3.7 support so we don't have to make changes to work around it. That version is EOL, and I already removed it elsewhere. Happy to make a patch for that if it would help. |
Go for it, I am happy to rebase my patch on that. We may want another patch to add 3.12 since that is the default in Fedora now. |
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.
Overall, this looks good. I am hoping the python version stuff will be able to be removed once my patch merges, and I have a few thoughts, largely around documentation.
4f13411
to
9482e98
Compare
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.
Super minor item or two, otherwise, looking good to me!
3ea4232
to
d947d91
Compare
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.
Couple minor points left over, but I think this is just about ready.
# Add the celery app as an extension to the flask app so it can be easily | ||
# accessed if a flask application factory pattern is used. | ||
# see https://flask.palletsprojects.com/en/2.3.x/patterns/celery/ for details. | ||
self.flask_app.extensions["celery"] = self.celery_app |
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.
This is a good improvement if it helps when the factory pattern is used, as I know a number of our applications do this currently.
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.
This provides a lot of flexibility, if you want to access the celery app from the flask app you can, or you can use the class attribute. This doesn't force a single implementation pattern, which I like.
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.
(praise) Really enjoying the README updates including link to relevant docs and installation/usage examples, really helpful!
It's really cool to see how nicely this works out. I'm approving although I'd like to see the rewording improvement suggested by Jay implemented if possible, as it also tripped me up on first read. Thanks for the patch!
@@ -28,6 +28,9 @@ install_requires = | |||
toolchest | |||
|
|||
[options.extras_require] | |||
celery = | |||
celery |
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.
(praise) I really like how self-contained the additions are, here and in the Pipfile, install instructions, etc. It works really neatly.
Adding celery_scaffold for use with flask based projects. It provides a way to configure celery for use, using the flask configuration files. It also provides a celery_app and a flask_app that can be used in your project. A base_scaffold was pulled out due to the celery worker assuming any 'app' attribute is of type Celery. The original app_scaffold is available to provide backward compatibility. It leverages the new base_scaffold and sets the 'app' attribute to flask_app to ensure existing use cases are handled. Signed-off-by: Jason Joyce <[email protected]>
d947d91
to
ea37426
Compare
Adding celery_scaffold for use with flask based projects. It provides a way to configure celery for use, using the flask configuration files. It also provides a celery_app and a flask_app that can be used in your project.
A base_scaffold was pulled out due to the celery worker assuming any 'app' attribute is of type Celery. The original app_scaffold is available to provide backward compatibility. It leverages the new base_scaffold and sets the 'app' attribute to flask_app to ensure existing use cases are handled.