[BEAM-4751] fix missing pylint3 check for io subpackage #5916
Conversation
|
Run Python PostCommit |
|
Run Python Dataflow ValidatesRunner |
superbobry
left a comment
superbobry
left a comment
There was a problem hiding this comment.
LGTM.
FYI #5869 replaces all NameError-based dispatch with past imports, including the ones you change in this PR.
| and self._start == other._start and self._end == other._end) | ||
|
|
||
| def __hash__(self): | ||
| return hash((type(self), self._start, self._end)) |
There was a problem hiding this comment.
Can you add self._split_freq to __eq__ and __hash__?
| return type(self) == type(other) and self.__dict__ == other.__dict__ | ||
|
|
||
| def __hash__(self): | ||
| return hash((type(self), self.__dict__)) |
There was a problem hiding this comment.
Does self.__dict__ work here for hashing?
| return self.stat == other.stat and self.getvalue() == self.getvalue() | ||
|
|
||
| def __hash__(self): | ||
| return hash((self.stat, self.getvalue())) |
There was a problem hiding this comment.
Can we use the dictionary self.stat?
sdks/python/apache_beam/io/vcfio.py
Outdated
| (other.name, other.genotype, other.phaseset, other.info)) | ||
|
|
||
| def __hash__(self): | ||
| return hash((self.name, self.genotype, self.phaseset, self.info)) |
There was a problem hiding this comment.
self.info is a dict. Is this a problem?
Fematich
force-pushed
the
io
branch
2 times, most recently
from
b0579e9 to
64298fa
Compare
| return type(self) == type(other) and self.__dict__ == other.__dict__ | ||
|
|
||
| def __hash__(self): | ||
| return hash((type(self), frozenset(self.__dict__.items()))) |
There was a problem hiding this comment.
I think this will be dead code. I suggest:
# To maintain compatibility with Python 3
__hash__ = None
| self.attributes == other.attributes) | ||
|
|
||
| def __hash__(self): | ||
| return hash((type(self), self.payload, self.attributes)) |
There was a problem hiding this comment.
Let's simplify this to hash(self.payload).
| def __eq__(self, other): | ||
| return self.stat == other.stat and self.getvalue() == self.getvalue() | ||
|
|
||
| def __hash__(self): |
There was a problem hiding this comment.
I think this will be dead code. I suggest:
# To maintain compatibility with Python 3
__hash__ = None
sdks/python/apache_beam/io/vcfio.py
Outdated
| vars(self) == vars(other)) | ||
|
|
||
| def __hash__(self): | ||
| return hash((type(self), vars(self))) |
There was a problem hiding this comment.
vars(self) is unhashable since it's a dict.
I think this would be dead code, so I suggest instead:
# To maintain compatibility with Python 3
__hash__ = None
sdks/python/apache_beam/io/vcfio.py
Outdated
| (other.name, other.genotype, other.phaseset, other.info)) | ||
|
|
||
| def __hash__(self): | ||
| return hash((self.name, self.genotype, |
There was a problem hiding this comment.
self.genotype is unhashable object. I think this would be dead code, so I suggest instead:
# To maintain compatibility with Python 3
__hash__ = None
| and self._end == other._end | ||
| and self._split_freq == other._split_freq) | ||
|
|
||
| def __hash__(self): |
There was a problem hiding this comment.
I think this will be dead code. I suggest:
# To maintain compatibility with Python 3
__hash__ = None
|
The test failure looks strange, perhaps something changed on master and we should rebase this? |
|
Exactly, apparently a change in the master in the time between me doing a rebase (before the push to the branch) and the test being run :-p. Small window ;-). Pushing the updated commit now. |
|
LGTM, thank you! |
5 participants
This pull request finishes the preparation of the io subpackage for Python 3 support. The work for io was started in #5715, however the py27-lint3 test was missing in tox.ini, resulting in some open issues and missing checks for new PRs.
The PR is part of a series in which all subpackages will be updated using the same approach.
This approach has been documented here and the first pull request in the series (Futurize coders subpackage) demonstrating this approach can be found at #5053.
R: @aaltay @tvalentyn @RobbeSneyders @charlesccychen @superbobry
Post-Commit Tests Status (on master branch)