-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Config Object #1552
base: master
Are you sure you want to change the base?
Config Object #1552
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1552 +/- ##
==========================================
- Coverage 95.59% 95.39% -0.21%
==========================================
Files 46 46
Lines 7061 6902 -159
==========================================
- Hits 6750 6584 -166
- Misses 311 318 +7
Continue to review full report at Codecov.
|
Implementation wise, this config object shouldn't be accessed as though this is a global or thread local object like from rq.config import get_configuration
serializer_class = get_configuration('serializer_class') I have plans to deprecate RQ's usage of thread locals. So this config object should be explicitly passed to worker = Worker(config=config)
worker.config.get('serializer_class') |
The problem of this is that we always need to use the config as an arg and so we could forget to implement it (For Example in functions of this project which are called from multiple other functions that don't need config now but in the future). That's because I used context manager and a global config stack to make the config accessable from everywhere. So @selwin can you just write me why you don't like globals? |
Correct, but the goal would be to put everything, including connection information into this config object.
I like it when everything is passed around so there's no ambiguity. There are many threads discussing why globals shouldn't be used, this is simply one of them https://stackoverflow.com/questions/19158339/why-are-global-variables-evil To me personally it's easier to maintain a code base where everything is mostly passed from one function to another. No surprises. |
Ok @selwin it's used as an arg, that is passed now and I implemented the cli. It is very ugly yet in the cli because there is no possibility to pass the config yet. |
rq/config.py
Outdated
|
||
|
||
class Config: | ||
def __init__(self, template=None, **kwargs): |
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'd like to have a more explicitly define config object so we can have an API like this:
config = get_config()
config.serializer_class
config.connection
The most important thing about this PR is that it breaks backwards compatibility. I'm not willing to make a backwards incompatible change without advanced warning. This means that |
docs/docs/index.md
Outdated
If you want to put the work on a specific queue, simply specify its name: | ||
|
||
```python | ||
q = Queue('low', connection=redis_conn) | ||
q = Queue('low') |
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 don't think we can get rid of connection
for these reasons:
- Storage must be explicitly defined
- I want users to be able to easily use RQ with default options without having to worry about config
So the minimum way to define a queue would be queue = Queue(connection=redis)
.
I don't know why this text fails. |
@rpkak I'm not sure. Does the test work on your local machine? |
No, it doesn't, but during manual testing, it worked fine. |
Hey @rpkak did you end up doing some more work on this? His has appeared in a couple of issues around here. A very trivial implementation for namespaces would be to read an envvar I was though of having a config being stored within the datastore in |
This PR adds a config object as a context manager.
The user can update the config by doing:
In RQ this data can be accessed using:
closes #1548
closes #1417