Skip to content

Ensure that UnrecognizedUnit.__eq__ never raises an exception. #7606

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
Merged
mhvk merged 1 commit into from
Sep 18, 2018
Merged

Ensure that UnrecognizedUnit.__eq__ never raises an exception. #7606

mhvk merged 1 commit into from
Sep 18, 2018

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Jun 29, 2018

fixes #7603

It just copies logic from UnitBase.__eq__.

@mhvk mhvk added this to the v2.0.8 milestone Jun 29, 2018
@astropy-bot
Copy link

astropy-bot bot commented Jun 29, 2018 •
edited
Loading

Hi there @mhvk 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@mhvk mhvk mentioned this pull request Jun 29, 2018
Closed
@mhvk mhvk requested a review from adrn June 29, 2018 16:29
@mhvk
Copy link
Contributor Author

mhvk commented Jun 29, 2018

Failure is a false-negative - URL issues (what else)

@pllim
Copy link
Member

pllim commented Jun 29, 2018

Argh... I thought we fixed it... Though this one looks like a different kind of failure:

>       return mod.open(file, flag, mode)
E       _gdbm.error: [Errno 11] Resource temporarily unavailable

/opt/python/cp36-cp36m/lib/python3.6/dbm/__init__.py:94: error

@mhvk mhvk force-pushed the unit-comparison-with-none branch from 8a827fb to ee43ce3 Compare July 1, 2018 15:48
mhvk
mhvk commented Jul 1, 2018
View reviewed changes
astropy/units/core.py
other = Unit(other, parse_strict='silent')
except (ValueError, UnitsError, TypeError):
return False
return NotImplemented
Copy link
Contributor Author

@mhvk mhvk Jul 1, 2018

Choose a reason for hiding this comment

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

This was not quite right before: the python standard is to return NotImplemented if one cannot compare to other sensibly - so that other gets a chance. If other then also returns NotImplemented, False is returned automatically. This might be useful for interaction with other unit packages (if they are aware of astropy units, and we're not aware of them).

@bsipocz bsipocz modified the milestones: v2.0.8, v2.0.9 Jul 31, 2018
@mhvk mhvk force-pushed the unit-comparison-with-none branch from ee43ce3 to 53944e7 Compare August 13, 2018 18:43
@mhvk
Copy link
Contributor Author

mhvk commented Aug 13, 2018

@taldcroft - any chance you might review this?

astrofrog
astrofrog approved these changes Sep 14, 2018
View reviewed changes
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, but probably should get a review from a maintainer more familiar with the units code.

taldcroft
taldcroft approved these changes Sep 17, 2018
View reviewed changes
Copy link
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.

Looks good to me.

@mhvk
Copy link
Contributor Author

mhvk commented Sep 18, 2018

Thanks both for the reviews. Merging...

@mhvk mhvk merged commit 99a7d9f into astropy:master Sep 18, 2018
@mhvk mhvk deleted the unit-comparison-with-none branch September 18, 2018 17:02
bsipocz pushed a commit that referenced this pull request Oct 5, 2018
@mhvk @bsipocz
Merge pull request #7606 from mhvk/unit-comparison-with-none
07bd731 
Ensure that UnrecognizedUnit.__eq__ never raises an exception.
bsipocz pushed a commit that referenced this pull request Oct 5, 2018
@mhvk @bsipocz
Merge pull request #7606 from mhvk/unit-comparison-with-none
23e8261 
Ensure that UnrecognizedUnit.__eq__ never raises an exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@astrofrog astrofrog astrofrog approved these changes

@taldcroft taldcroft taldcroft approved these changes

@adrn adrn Awaiting requested review from adrn

Assignees

No one assigned

Labels

Bug units

Projects

None yet

Milestone

v2.0.9

Development

Successfully merging this pull request may close these issues.

Unit equality comparison with None raises TypeError for UnrecognizedUnit

5 participants

@mhvk @pllim @astrofrog @taldcroft @bsipocz