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.

Sign up for GitHub

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

Jump to bottom

MAINT: Update use_header handling #1149

Merged
merged 5 commits into from Oct 18, 2023
Merged

MAINT: Update use_header handling #1149

merged 5 commits into from Oct 18, 2023

Conversation

aburrell
Copy link
Member

@aburrell aburrell commented Oct 17, 2023 •
edited

Description

Partially addresses #1020 by removing use_header as a standard kwarg from Instrument.load. Updated the DeprecationWarning and removed unnecessary use of use_header from the test suite.

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality
    to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

import pysat

inst = pysat.Instrument('pysat', 'testing')
# This will not raise a deprecation warning
inst.load(2009, 1)

# This will raise a deprecation warning, but behave the same way
inst.load(2009, 2, use_header=True)

# This will raise a deprecation warning, and go back to the old behaviour
inst.load(2009, 1, use_header=False)

Test Configuration:

  • Operating system: OS X Big Sur
  • Version number: Python 3.9
  • Any details about your local setup that are relevant

Checklist:

  • Make sure you are merging into the develop (not main) branch
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • Add a note to CHANGELOG.md, summarizing the changes

If this is a release PR, replace the first item of the above checklist with the release
checklist on the wiki: https://github.com/pysat/pysat/wiki/Checklist-for-Release

@aburrell
MAINT: removed use_header
ce0f752 
Removed `use_header` as a standard kwarg for `load`.  Adjusted deprecation warning to notify user of the end of support for the optional kwarg.
@aburrell
DOC: updated basic tutorial
588a735 
Updated basic tutorial by changing the description of the old `use_header` kwarg.
@aburrell
TST: removed use_header kwarg
28cfb88 
Removed the `use_header` kwarg from the test suite and updated the deprecation test.
@aburrell
DOC: updated changelog
2f4f07e 
Added a description of the changes to the changelog.
@aburrell
Copy link
Member Author

Appears that there's no new uncovered lines, just changes due to changes in the number of lines per file.

@aburrell aburrell added this to the 3.2.0 Release milestone Oct 17, 2023
@aburrell aburrell linked an issue Oct 17, 2023 that may be closed by this pull request
MAINT: Remove the use_header kwarg #1020
Open
2 tasks
jklenzing
jklenzing reviewed Oct 17, 2023
View reviewed changes
Copy link
Member

@jklenzing jklenzing left a comment

Choose a reason for hiding this comment

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

Testing this against other packages that still set use_header on instrument instantiation, line 367 of _instrument.py produces an error when checking for other kwargs.

        # Test for user supplied keys that are not used
        missing_keys = []
        for custom_key in kwargs:
            if custom_key not in saved_keys and (custom_key not in exp_keys):
                missing_keys.append(custom_key)
    
        if len(missing_keys) > 0:
>           raise ValueError('unknown keyword{:s} supplied: {:}'.format(
                '' if len(missing_keys) == 1 else 's', missing_keys))
E           ValueError: unknown keyword supplied: ['use_header']

../pysat/pysat/_instrument.py:367: ValueError

Probably need to check here as well

@aburrell
BUG: use_header can be used at init
9ca8e68 
Make sure the `use_header` kwarg is allowed for `load` at initialization.
@aburrell
Copy link
Member Author

This would only appear if they apply use_header at Instrument initiation. I think I fixed it, but my local package structure is to messed up for local tests.

@jklenzing
Copy link
Member

Looks good. Works with pysatNASA tests now.

@aburrell aburrell merged commit 189e5e4 into develop Oct 18, 2023
15 of 17 checks passed
@aburrell aburrell deleted the update_use_header_default branch October 18, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklenzing jklenzing jklenzing approved these changes

Assignees
No one assigned
Labels
breaking change
Projects
None yet
Milestone
3.2.0 Release
Development

Successfully merging this pull request may close these issues.

MAINT: Remove the use_header kwarg
2 participants
@aburrell @jklenzing