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

Change import to have configurable prefixes via set_options #460

Merged
merged 4 commits into from
Mar 24, 2022

Conversation

aulemahal
Copy link
Contributor

@aulemahal aulemahal commented Mar 14, 2022

Change Summary

Restructures the import of the default attribute names and prefixes in source.py so that they are actually configurable by patching the variables in utils.py.

Related issue number

No issue sorry.

The problem was the source.py imported the variables directly, this copying the python reference to the string. As strings are non-mutable, someone that would want to customize the attribute names has to re-set the variable in the utils module (I think this is called "monkey-patching" ?). But that doesn't update the version in source.py. Here, the solution is to import the whole utils module object in source, so that the strings are queried as members of the module everytime they are needed, not only a import time.

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable

@aulemahal aulemahal requested a review from andersy005 as a code owner March 14, 2022 19:58
@andersy005
Copy link
Member

I didn't realize users would need to override these variables. I'd like to suggest a different approach that exposes these variables and (other future configuration) as options that users can easily manipulate. Using donfig would simplify this process and make it easy to modify these configurations. Let me know what you think.

@aulemahal
Copy link
Contributor Author

Oh! The way they were declared as constants made me think that was the goal. In our case, we found intake_esm_attrs to be kinda long to type. And, as we are subclassing the catalog class, the user has not explicit knowledge that intake_esm is used, which makes the name a bit weird. We are overriding it with "cat" for now.

I agree that this is not super clean. But there are only 3 variables... If you plan to add more user-controllable stuff, then yes an external package could be useful. For now, I feel it is not needed? I'm a dev at xclim and when we encountered the problem, we decided to go with the set_options function/context solution (like xarray), because it needed no extra package.

@andersy005
Copy link
Member

If you have time, let's go with the options dict approach used by xarray for now ( I don't envision other user-configurable settings in the future).

@aulemahal
Copy link
Contributor Author

@andersy005 Last commit pushed a simplified version of xarray's mechanic for options! I added the class to the API page and mentioned it in the docstring of to_dataset_dict.

Copy link
Member

@andersy005 andersy005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, @aulemaha! Thank you...

@andersy005 andersy005 added the enhancement Issues that are found to be a reasonable candidate feature additions label Mar 24, 2022
@andersy005 andersy005 changed the title Change import to have configurable prefixes Change import to have configurable prefixes via set_options Mar 24, 2022
@andersy005 andersy005 merged commit a5c6ef0 into intake:main Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues that are found to be a reasonable candidate feature additions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants