Skip to content

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

Merged
mhvk merged 1 commit into
astropy:masterfrom
mhvk:unit-comparison-with-none
Sep 18, 2018
Merged

Ensure that UnrecognizedUnit.__eq__ never raises an exception.#7606
mhvk merged 1 commit into
astropy:masterfrom
mhvk:unit-comparison-with-none

Conversation

@mhvk

@mhvk mhvk commented Jun 29, 2018

Copy link
Copy Markdown
Contributor

fixes #7603

It just copies logic from UnitBase.__eq__.

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

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

Copy link
Copy Markdown

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

mhvk commented Jun 29, 2018

Copy link
Copy Markdown
Contributor Author

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

@pllim

pllim commented Jun 29, 2018

Copy link
Copy Markdown
Member

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
Comment thread astropy/units/core.py
other = Unit(other, parse_strict='silent')
except (ValueError, UnitsError, TypeError):
return False
return NotImplemented

@mhvk mhvk Jul 1, 2018

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.

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

mhvk commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

@taldcroft - any chance you might review this?

astrofrog
astrofrog approved these changes Sep 14, 2018
View reviewed changes

@astrofrog astrofrog 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.

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

@taldcroft taldcroft 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.

Looks good to me.

@mhvk

mhvk commented Sep 18, 2018

Copy link
Copy Markdown
Contributor Author

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