-
-
Notifications
You must be signed in to change notification settings - Fork 400
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
Update handling configuration items in template #2341
base: main
Are you sure you want to change the base?
Conversation
Previously the template core module read the values of the configuration items at class creation and stored them in class attributes. This meant that changing the configuration items at runtime had no effect. Now the template implements private properties that return the current configuration item value, or the class (or instance) attribute value if the user has changed it. This enables the users to use the full functionality of the `astropy` configuration system, but users not familiar with it can still override it entirely through the attributes.
The template core module allows users to set configuration items in two ways, either through the `astropy` configuration system or through class or instance attributes. The attribute names are no longer in capital letters because the convention is to reserve such names for constants, and the configuration item names now match the attribute names.
Codecov Report
@@ Coverage Diff @@
## main #2341 +/- ##
==========================================
+ Coverage 63.41% 63.43% +0.01%
==========================================
Files 131 131
Lines 17043 17051 +8
==========================================
+ Hits 10808 10816 +8
Misses 6235 6235
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@@ -36,15 +36,27 @@ | |||
@async_to_sync | |||
class TemplateClass(BaseQuery): | |||
|
|||
def __init__(self, url='', timeout=None): | |||
self.url = url # A falsy default that cannot be mistaken for a valid value. | |||
self.timeout = timeout # Use `None` as default if the falsy value could be valid. |
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'm not sure our users will all be familiar with the term "falsy" - maybe clarify, e.g., ("falsy" means in this case bool(url) is False)
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.
The target audience for these comments are developers, but I added an explanation nonetheless.
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.
our developers are not necessarily experienced, and in particular, not necessarily experienced in python (we have many people coming from Java backgrounds, for example)
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.
But then "falsy" is not a python specific term :) Anyway, I don't think it matters either way. Also, in my experience, many new modules are taking another similar module as a template rather than the "template" one, so we may even want to rethink the purpose of the template.
The template now recommends implementing an `__init__()` function that allows the users to conveniently override the `astropy` configuration system.
6a4027a
to
1ffb0d9
Compare
Does this pull request need a change log entry or has the |
I don't think it needs one, but I leave the label off as a reminder for release time to revisit, if this next release indeed accretes multiple of the smaller infra refracture we may want to add one entry to cover those changes. |
I have added a section to the opening post that explains the motivation for the changes this pull request proposes. |
Three updates have been made to the subpackage template.
astropy
configuration system at runtime, or to overwrite it entirely using class or instance attributes. See Fix changingsimbad
config items at runtime #2292 for an example of what this means in practice.__init__()
method that would allow the user to conveniently create instances with non-default configuration attributes.These updates, if merged, would specify the recommended API for any new subpackages, but would also serve as a guideline for updating the existing subpackages to converge on a more uniform API.
Motivation
The
astropy
configuration system provides many useful features, for example:set_temp()
context manager makes it convenient to test different configuration item values,astroquery
should therefore use theastropy
configuration system as much as possible, but users not familiar with it would benefit if an alternative way of changing configurable values would also be present.The current template recommends creating class attributes which take their values from the configuration system, but the class attributes are created when the class is created, which happens when the module containing the class is executed for being imported. This means that changing the configuration items at runtime has no effect on the class attributes, which is the cause of many bugs in
astroquery
(see #544 and #2291, or for specific examples see e.g. #493 and #2099).The new template proposed in this pull request would recommend the query classes to implement for every configuration item a corresponding instance attribute and private property. The private property would return the value of the instance attribute if a non-default value has been specified through an optional keyword argument to the
__init__()
method at instance creation or directly at a later time, otherwise it would return the value from the configuration system. This has the following benefits:astropy
configuration system can use all of its features, including changing values at runtime,