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

Added get_options method #5716

Merged
merged 8 commits into from
Sep 4, 2021
Merged

Added get_options method #5716

merged 8 commits into from
Sep 4, 2021

Conversation

pkopparla
Copy link
Contributor

@pkopparla pkopparla commented Aug 19, 2021

  • Closes Show what options are enabled. #5698
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

Added a get_options method under core/options.py. At the moment it returns the OPTIONS dictionary as is to keep it very simple, if a formatted print statement with text is desirable I can try to add it.

Edit: Some tests are failing, but they are unrelated to my changes. Not sure how to address the errors.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 19, 2021

Unit Test Results

         6 files  ±0           6 suites  ±0   50m 28s ⏱️ ±0s
16 227 tests ±0  14 493 ✔️ ±0  1 734 💤 ±0  0 ❌ ±0 
90 558 runs  ±0  82 382 ✔️ ±0  8 176 💤 ±0  0 ❌ ±0 

Results for commit 9dbeb6b. ± Comparison against base commit 9dbeb6b.

♻️ This comment has been updated with latest results.

@Illviljan
Copy link
Contributor

The returned dict should be read only, otherwise you can bypass all the validation checks that's done in set_options. I think you can use FrozenDict for that:

def FrozenDict(*args, **kwargs) -> Frozen:

Not sure if FrozenDict propagates the typing though, once OPTIONS is typed.

We probably need to make sure that get_options is updated if set_options has been used. A test for that would be nice.

@max-sixty
Copy link
Collaborator

This looks good., thanks @pkopparla Thanks for reviewing @Illviljan .

Agree re a test — at least to just test it runs, albeit trivial.

FrozenDict would be better, though I think fine to do in another PR. We could return dict(OPTIONS) if we're worried people will attempt to set invalid values.

@pkopparla
Copy link
Contributor Author

pkopparla commented Aug 20, 2021

Thank you @Illviljan @max-sixty
I've changed the return type to FrozenDict and added a check to make sure that set_options changes are reflected in get_options.

Edit: All unit tests are passing on my local PC but there's one failure in the github checks.

xarray/core/options.py Outdated Show resolved Hide resolved
doc/whats-new.rst Outdated Show resolved Hide resolved
Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks @pkopparla This is a great improvement!

doc/whats-new.rst Outdated Show resolved Hide resolved
xarray/core/options.py Outdated Show resolved Hide resolved
xarray/core/options.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks @pkopparla

I'll merge in a couple of days if there are no more comments

@dcherian dcherian added the plan to merge Final call for comments label Aug 23, 2021
@pkopparla
Copy link
Contributor Author

Awesome, thanks @dcherian and all the reviewers!

@max-sixty max-sixty merged commit 9dbeb6b into pydata:main Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show what options are enabled.
4 participants