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

Return None for unknown pieces of metadata #5126

Merged
merged 1 commit into from Apr 1, 2021
Merged

Return None for unknown pieces of metadata #5126

merged 1 commit into from Apr 1, 2021

Conversation

dstansby
Copy link
Member

xref #5124 (comment). I have always wondered why these return default values when they are unknown. Instead, return None. This is a breaking change, but I think fine as long as we document it well? Opinions very welcome.

@dstansby dstansby added this to the 3.0 milestone Mar 22, 2021
@dstansby dstansby requested a review from Cadair March 22, 2021 15:22
@nabobalis nabobalis added the No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) label Mar 22, 2021
@dstansby dstansby mentioned this pull request Mar 22, 2021
Closed
@dstansby dstansby force-pushed the map-defaults branch 2 times, most recently from 71ae874 to 3ef9176 Compare March 28, 2021 11:26
@dstansby dstansby marked this pull request as ready for review March 28, 2021 11:26
@dstansby dstansby requested a review from a team as a code owner March 28, 2021 11:26
@dstansby dstansby added the map Affects the map submodule label Mar 28, 2021
@Cadair
Copy link
Member

Cadair commented Mar 29, 2021

The CI needs fixing but the changes look good.

@dstansby
Copy link
Member Author

Anyone got any ideas why my attempt at ignoring the warning didn't work?

@dstansby dstansby force-pushed the map-defaults branch 3 times, most recently from 8328f67 to b56c04d Compare March 30, 2021 17:02
@dstansby dstansby force-pushed the map-defaults branch 2 times, most recently from 7db13d0 to 6407a4f Compare March 31, 2021 16:37
@dstansby
Copy link
Member Author

@Cadair can you remember what we decided to do if WAVEUNIT Isn't present but WAVELNTH is? Should map.wavelength return:

  1. None
  2. A bare number
  3. A Quantity with u.dimensionless units?

@Cadair
Copy link
Member

Cadair commented Mar 31, 2021

I would go for quantity with dimensionless units. I think thats what happens at the moment in this code anyway.

@nabobalis nabobalis merged commit 440ae82 into sunpy:master Apr 1, 2021
@dstansby dstansby deleted the map-defaults branch April 1, 2021 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cadair Cadair Cadair approved these changes

@nabobalis nabobalis nabobalis approved these changes

Assignees
No one assigned
Labels
map Affects the map submodule No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers)
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dstansby @Cadair @nabobalis