Skip to content

Expose Header.strip as a public method - #11174

Merged
embray merged 2 commits into
astropy:masterfrom
saimn:fits-strip
Dec 31, 2020
Merged

Expose Header.strip as a public method#11174
embray merged 2 commits into
astropy:masterfrom
saimn:fits-strip

Conversation

@saimn

@saimn saimn commented Dec 17, 2020

Copy link
Copy Markdown
Contributor

This method is currently private though it can be useful to remove the most common structural keywords.

@saimn saimn added the io.fits label Dec 17, 2020
@saimn saimn added this to the v4.3 milestone Dec 17, 2020
@saimn
saimn requested a review from embray December 17, 2020 18:35
pllim
pllim reviewed Dec 17, 2020
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was there a reason for it to be private in the first place? Is there a reason why it should be public now? Will people misuse it and then think it is a bug instead of a feature?

@saimn

saimn commented Dec 17, 2020

Copy link
Copy Markdown
Contributor Author

Is there a reason why it should be public now?

Because it's useful to remove the FITS structural keywords before updating another header with the stripped one.

I don't see how it could be a "bug". Also it's currently possible to call .copy(strip=True) which uses this method, it would just be useful to call it in-place.

@embray

embray commented Dec 31, 2020

Copy link
Copy Markdown
Member

LGTM. I think maybe a nice follow-up would be to change the list of keywords to remove to a global list (including some "patterns" e.g. for NAXIS type keywords) and allow this method to take a list of additional keywords to either remove or keep besides the defaults.

@embray
embray merged commit de7420e into astropy:master Dec 31, 2020
@saimn
saimn deleted the fits-strip branch January 17, 2021 22:01
pllim added a commit to pllim/astropy that referenced this pull request Mar 24, 2021
@pllim
Remove test added in astropy#11174
332c50c
pllim added a commit to pllim/astropy that referenced this pull request Mar 25, 2021
@pllim
Remove test added in astropy#11174
984ca28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
+1 more reviewer
@embray embray embray approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

io.fits Refactoring

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

3 participants

@saimn @embray @pllim