Skip to content

Fixes SyntaxWarnings from Python 3.8#9225

Merged
bsipocz merged 1 commit intoastropy:masterfrom
MSeifert04:fix-some-python-3.8-warnings
Sep 14, 2019
Merged

Fixes SyntaxWarnings from Python 3.8#9225
bsipocz merged 1 commit intoastropy:masterfrom
MSeifert04:fix-some-python-3.8-warnings

Conversation

@MSeifert04
Copy link
Copy Markdown
Contributor

The two comparisons usings "is" trigger the warning and because these are strings they can be replaced by "==".

The one test in wcsaxes looks like a real bug where the missing "," was interpreted as function call on a tuple.

Not sure about the milestone.

The two comparisons usings "is" trigger the warning and because these are
strings they can be replaced by "==".

The one test in wcsaxes looks like a real bug where the missing ","
was interpreted as function call on a tuple.
return [('freq', 0, 'value'),
('time', 0, 'mjd'),
('celestial', 0, 'spherical.lon.degree')
('celestial', 0, 'spherical.lon.degree'),
Copy link
Copy Markdown
Member

@pllim pllim Sep 13, 2019

Choose a reason for hiding this comment

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

Wait a minute... how did this not fail CI (before this fix)?

Copy link
Copy Markdown
Contributor Author

@MSeifert04 MSeifert04 Sep 13, 2019

Choose a reason for hiding this comment

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

Because the property is never called?! 😅

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.

@astrofrog , looks like adding a test to cover this is in order?

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.

Test coverage fail....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this really needs a test since it's just a test-class that needs to satisfy the abstract properties of BaseLowLevelWCS that might not be even relevant for the test.

That's why I just fixed the SyntaxWarning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Given that the PR is merged now, should we open an issue for this issue?

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.

Indeed, @MSeifert04 ! Do you want the honor? 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

okay

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See #9241

@larrybradley larrybradley modified the milestones: v4.0, v3.2.2 Sep 13, 2019
@larrybradley
Copy link
Copy Markdown
Member

I changed the milestone to 3.2.2 since these appear to be bugfixes.

@mhvk mhvk mentioned this pull request Sep 13, 2019
self._represent_as_dict_attrs += ('mask',)

elif method is 'null_value':
elif method == 'null_value':
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice catch. Coincidentally, also caught by @taldcroft in a PR by me: #8998 (comment)

Copy link
Copy Markdown
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Good from my perspective.

@bsipocz bsipocz merged commit d80aa83 into astropy:master Sep 14, 2019
@bsipocz
Copy link
Copy Markdown
Member

bsipocz commented Sep 14, 2019

Thank you @MSeifert04!

@MSeifert04 MSeifert04 deleted the fix-some-python-3.8-warnings branch September 14, 2019 20:57
bsipocz added a commit that referenced this pull request Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants