-
-
Notifications
You must be signed in to change notification settings - Fork 463
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
Better config helpers API #602
Conversation
* Move `update_config` and `setup_config` helper methods from module and place them on config object itself. * Rename both helper context managers to `patch` and use `clear` argument of `patch` to replace the whole config. * Keep `setup` as simple method only (also straight on `config`)
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.
❤️ So much nicer!
Just a couple of tiny comments
nameko/__init__.py
Outdated
@pytest.yield_fixture | ||
def memory_rabbit_config(): | ||
with nameko.config.patch({"AMQP_URI": "memory://"}): | ||
yield |
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.
It's a little confusing that this fixture is defined but not used. I think you can drop this and just demonstrate the decorator usage.
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.
👍
nameko/__init__.py
Outdated
|
||
""" | ||
|
||
config = self.data |
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.
Might be clearer to call this data
. I was briefly confused and thought the implementation below was writing to the global config var.
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.
👍
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.
Nice!
I found that
patch
is a better fit for the context manager name as it suggests that the config changes are returned back at exit:When migrating a few services to global config I learned that having also a decorator for setting up test function context config would be very handy:
I also placed these helpers straight on config object having them available straight away and making the config API clearer.