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

Update handling configuration items in template #2341

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eerovaher
Copy link
Member

@eerovaher eerovaher commented Mar 28, 2022

Three updates have been made to the subpackage template.

  1. It now recommends implementing private properties so that it would be possible to use the astropy configuration system at runtime, or to overwrite it entirely using class or instance attributes. See Fix changing simbad config items at runtime #2292 for an example of what this means in practice.
  2. The template attributes that would overwrite the configuration items are no longer named as if they are constants, and the configuration item names match the attribute names.
  3. It is now recommended to implement an __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:

  • users can make permanent changes through editing the configuration file,
  • the set_temp() context manager makes it convenient to test different configuration item values,
  • configuration item values and types are verified, so attempting to set an invalid value fails immediately.

astroquery should therefore use the astropy 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:

  • users familiar with the astropy configuration system can use all of its features, including changing values at runtime,
  • users not familiar with the configuration system can still make changes in a simple way,
  • developers writing code that needs to access configurable values can always do so in a uniform way through the private property, which will take care of selecting either the instance attribute or the configuration item without needless code duplication or harming code readability.

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
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #2341 (6a4027a) into main (c0700b8) will increase coverage by 0.01%.
The diff coverage is 83.33%.

❗ Current head 6a4027a differs from pull request most recent head 1ffb0d9. Consider uploading reports for the commit 1ffb0d9 to get more accurate results

@@            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              
Impacted Files Coverage Δ
astroquery/template_module/core.py 65.21% <83.33%> (+4.56%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@bsipocz bsipocz added this to the v0.4.7 milestone Mar 28, 2022
@@ -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.
Copy link
Contributor

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)

Copy link
Member Author

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.

Copy link
Contributor

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)

Copy link
Member

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.
@eerovaher
Copy link
Member Author

Does this pull request need a change log entry or has the no-changelog-entry-needed label been forgotten?

@bsipocz
Copy link
Member

bsipocz commented Mar 29, 2022

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.

@eerovaher eerovaher changed the title Update template Update handling configuration items in template Mar 29, 2022
@eerovaher
Copy link
Member Author

I have added a section to the opening post that explains the motivation for the changes this pull request proposes.

@bsipocz bsipocz removed this from the v0.4.7 milestone Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants