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.

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

Bug comparing ComparingObject to an object without __dict__ #2220

Merged
merged 3 commits into from Oct 12, 2018

Conversation

Projects
None yet
3 participants
@dsentinel
Contributor

dsentinel commented Sep 25, 2018

Issue:

Comparing ComparingObjects to random objects with == / __eq__ should return False not raise and excption.

Actual case:

If you have 2 Channels one with a valid Equipment and the other with None
chan1 == chan2
raises

  File "/Users/lloyd/code/obspy/obspy/core/util/base.py", line 519, in __ne__
    return not self.__eq__(other)
  File "/Users/lloyd/code/obspy/obspy/core/util/base.py", line 516, in __eq__
    return self.__dict__ == other.__dict__
AttributeError: 'int' object has no attribute '__dict__'

The first commit only has the tests, you can pull and exersize.

Solution:

At first I was worried about this solution breaking comparisons with other types that were compatible, e.g. CustomComplex == complex, but those kinds of types don't inherit from ComparingObject, and those seem to inherit from their compatible types... and the tests passed.

@dsentinel

This comment has been minimized.

Show comment
Hide comment
@dsentinel

dsentinel Sep 25, 2018

Contributor

Is there a python3 PR tag or #TODO: comment for refactors of code that can be cleaner when not supporting python2. I come across these and feel like I should do something while there in my head.

Contributor

dsentinel commented Sep 25, 2018

Is there a python3 PR tag or #TODO: comment for refactors of code that can be cleaner when not supporting python2. I come across these and feel like I should do something while there in my head.

@dsentinel dsentinel requested review from krischer and QuLogic Sep 28, 2018

import shutil
import unittest
from obspy.core.compatibility import mock
from obspy.core.util.base import (NamedTemporaryFile, get_dependency_version,
download_to_file, sanitize_filename,
create_empty_data_chunk)
create_empty_data_chunk, ComparingObject)

This comment has been minimized.

@QuLogic

QuLogic Sep 29, 2018

Member

This sorts at the very beginning of the import list.

@QuLogic

QuLogic Sep 29, 2018

Member

This sorts at the very beginning of the import list.

This comment has been minimized.

@dsentinel

dsentinel Oct 1, 2018

Contributor

Thanks for the review.
Sorry, I don't understand what this comment means. Are you suggesting a change?

@dsentinel

dsentinel Oct 1, 2018

Contributor

Thanks for the review.
Sorry, I don't understand what this comment means. Are you suggesting a change?

This comment has been minimized.

@megies

megies Oct 5, 2018

Member

I don't think we ever bothered to enforce alphabetical order inside import attribute lists, we just enforce alphabetical order in the module names being imported from. So if this is what @QuLogic meant I think it can stay as is.

@megies

megies Oct 5, 2018

Member

I don't think we ever bothered to enforce alphabetical order inside import attribute lists, we just enforce alphabetical order in the module names being imported from. So if this is what @QuLogic meant I think it can stay as is.

This comment has been minimized.

@dsentinel

dsentinel Oct 5, 2018

Contributor

oh, gotcha, thanks!

@dsentinel

dsentinel Oct 5, 2018

Contributor

oh, gotcha, thanks!

@dsentinel dsentinel self-assigned this Oct 1, 2018

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 5, 2018

Member

Is there a python3 PR tag or #TODO: comment for refactors of code that can be cleaner when not supporting python2. I come across these and feel like I should do something while there in my head.

I'd propose to just keep a list of things in a special ticket, see #2228

Member

megies commented Oct 5, 2018

Is there a python3 PR tag or #TODO: comment for refactors of code that can be cleaner when not supporting python2. I come across these and feel like I should do something while there in my head.

I'd propose to just keep a list of things in a special ticket, see #2228

@megies

megies approved these changes Oct 5, 2018

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 5, 2018

Member

Hmm.. dunno, maybe this should even be considered a bug and go into maintenance instead.. It would be really weird if we were breaking somebody's code by not raising an exception anymore in a place were none is expected to raise, IMHO

Member

megies commented Oct 5, 2018

Hmm.. dunno, maybe this should even be considered a bug and go into maintenance instead.. It would be really weird if we were breaking somebody's code by not raising an exception anymore in a place were none is expected to raise, IMHO

@dsentinel

This comment has been minimized.

Show comment
Hide comment
@dsentinel

dsentinel Oct 5, 2018

Contributor

Is there a python3 PR tag or #TODO: comment for refactors of code that can be cleaner when not supporting python2. I come across these and feel like I should do something while there in my head.

I'd propose to just keep a list of things in a special ticket, see #2228

Thats good a good idea. Works well for general changes.
I was thinking more along the lines of specific code changes, in the code, or with line numbers
like these lines should look like this after py2 drop.
...
Perhaps just a new branch would be the way to go.

Contributor

dsentinel commented Oct 5, 2018

Is there a python3 PR tag or #TODO: comment for refactors of code that can be cleaner when not supporting python2. I come across these and feel like I should do something while there in my head.

I'd propose to just keep a list of things in a special ticket, see #2228

Thats good a good idea. Works well for general changes.
I was thinking more along the lines of specific code changes, in the code, or with line numbers
like these lines should look like this after py2 drop.
...
Perhaps just a new branch would be the way to go.

@dsentinel

This comment has been minimized.

Show comment
Hide comment
@dsentinel

dsentinel Oct 5, 2018

Contributor

Hmm.. dunno, maybe this should even be considered a bug and go into maintenance instead.. It would be really weird if we were breaking somebody's code by not raising an exception anymore in a place were none is expected to raise, IMHO

I definitely consider this a bug.

I pulled from master as the maintenance branch seemed to have fallen behind.
I can rebase from master if appropriate. I need this to in a project to publish, so I'd like to see it in 1.1.0 or at least master as soon as can be.

I can't imagine anyone relying on this behavior.

Contributor

dsentinel commented Oct 5, 2018

Hmm.. dunno, maybe this should even be considered a bug and go into maintenance instead.. It would be really weird if we were breaking somebody's code by not raising an exception anymore in a place were none is expected to raise, IMHO

I definitely consider this a bug.

I pulled from master as the maintenance branch seemed to have fallen behind.
I can rebase from master if appropriate. I need this to in a project to publish, so I'd like to see it in 1.1.0 or at least master as soon as can be.

I can't imagine anyone relying on this behavior.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 8, 2018

Member

I pulled from master as the maintenance branch seemed to have fallen behind.
I can rebase from master if appropriate. I need this to in a project to publish, so I'd like to see it in 1.1.0 or at least master as soon as can be.

See branching model, changes in maintenance branch are bug fix only and will end up in release version 1.1.1 eventually, master branch changes will end up in release 1.2.0. If it is merged into maintenance branch it can be right away also in master branch, we regularly merge maintenance into master anyway and if there is a need to have it in master we can do it any time.

Member

megies commented Oct 8, 2018

I pulled from master as the maintenance branch seemed to have fallen behind.
I can rebase from master if appropriate. I need this to in a project to publish, so I'd like to see it in 1.1.0 or at least master as soon as can be.

See branching model, changes in maintenance branch are bug fix only and will end up in release version 1.1.1 eventually, master branch changes will end up in release 1.2.0. If it is merged into maintenance branch it can be right away also in master branch, we regularly merge maintenance into master anyway and if there is a need to have it in master we can do it any time.

@dsentinel dsentinel changed the base branch from master to maintenance_1.1.x Oct 8, 2018

@dsentinel

This comment has been minimized.

Show comment
Hide comment
@dsentinel

dsentinel Oct 8, 2018

Contributor

rebased and PR'ed against maintenance_1.1.x

Contributor

dsentinel commented Oct 8, 2018

rebased and PR'ed against maintenance_1.1.x

@megies megies added this to the 1.1.1 milestone Oct 10, 2018

@megies megies added the bug label Oct 10, 2018

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 10, 2018

Member

Although it might not affect most people, maybe it'd still be good to have a one line changelog message, just for the sake of completeness.

Otherwise good to merge. 👍

Member

megies commented Oct 10, 2018

Although it might not affect most people, maybe it'd still be good to have a one line changelog message, just for the sake of completeness.

Otherwise good to merge. 👍

@dsentinel

This comment has been minimized.

Show comment
Hide comment
@dsentinel

dsentinel Oct 11, 2018

Contributor

Changelogged.
I'm not allowed to merge :/

Contributor

dsentinel commented Oct 11, 2018

Changelogged.
I'm not allowed to merge :/

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 12, 2018

Member

I'm not allowed to merge :/

Hmm.. but it has approving reviews.. dunno, but I'll merge it anyways now. ;-)

Member

megies commented Oct 12, 2018

I'm not allowed to merge :/

Hmm.. but it has approving reviews.. dunno, but I'll merge it anyways now. ;-)

@megies megies merged commit a866c4e into maintenance_1.1.x Oct 12, 2018

1 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details

@megies megies deleted the bug_comparing_to_none branch Oct 12, 2018

@dsentinel

This comment has been minimized.

Show comment
Hide comment
@dsentinel

dsentinel Oct 17, 2018

Contributor

Thanks!

Contributor

dsentinel commented Oct 17, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment