146 update python dependencies #147
Conversation
Change appveroy python to 3.8
Codecov Report
@@ Coverage Diff @@
## master #147 +/- ##
==========================================
+ Coverage 86.35% 86.42% +0.06%
==========================================
Files 25 25
Lines 2594 2607 +13
==========================================
+ Hits 2240 2253 +13
Misses 354 354
Continue to review full report at Codecov.
|
vistautils/range.py
Outdated
| @@ -687,11 +687,11 @@ def range_containing(self, value: T) -> Optional[Range[T]]: | |||
| raise NotImplementedError() | |||
|
|
|||
| @abstractmethod | |||
| def range_enclosing_range(self, value: Range[T]) -> Optional[Range[T]]: | |||
| def range_enclosing_range(self, rng: Range[T]) -> Optional[Range[T]]: | |||
There was a problem hiding this comment.
Was there a compelling test failure that required changing the parameter names? Otherwise given this is a published packaged I'd refrain from doing so incase anyone is passing in parameters by keyword.
There was a problem hiding this comment.
There was a pylint error complaining that the child classes (e.g. _SortedDictRangeSet) had function parameters which overwrote the abstract method's names (in this case, _SortedDictRangeSet.range_enclosing_range() used rng as a parameter where the abstract class used value. In hindsight, I don't think this should require a change at all since it didn't cause an error in the Python 3.6 environment I set up for testing. I think this is something which was ignored prior but is now raised as an issue since I updated the pylint version to work with Python 3.8
There was a problem hiding this comment.
Ah! Well ok, this is probably something that should change but doing so is breaking compatibility so let's just ignore the error for now.
There was a problem hiding this comment.
Ok, I can revert the classes to how they were before and ignore the errors.
There was a problem hiding this comment.
The parameters are now back to how they were before and the corresponding child class functions are set up to ignore the specific error pylint was raising (# pylint: disable=W0237)
| @@ -28,7 +28,7 @@ def main(params: Parameters): | |||
| with byte_key_value_sink_from_params(params, eval_context=locals()) as out: | |||
| for input_path in input_paths: | |||
| with KeyValueSource.zip_bytes_source(input_path) as inp: | |||
| for key in inp.keys(): | |||
| for key in inp.items(): | |||
There was a problem hiding this comment.
This conversion seems wrong? .keys() is not replaced by .items()?
There was a problem hiding this comment.
You're correct, it slipped my mind before. I realized this error earlier today and just forgot to draft a fix. pylint prefers .items() to .keys() with the newer version, so I'll change it to the following:
for key, _ in inp.items()
I'm not really sure why pylint prefers .keys() to .items() though
There was a problem hiding this comment.
If you just want to iterate over the keys you don't need to call any method. Dictionaries iterate over their keys by default.
Actually using .keys() may be correct here. I'd need to review the KeyValueSource more closely.
There was a problem hiding this comment.
That would work too, and I think that would look cleaner than the alternative.
There was a problem hiding this comment.
Right ok, looks like the KeyValueSource is backed in such a way that the .keys() should be the default iterator for the object.
Ignores naming specifications for some testing variables, and shifts from using .keys() to .items() for dictionary key iteration.
Update setup.py, add feature breakdown for Python 3.8 compatibility, and drop unused options in .pylintrc
sidharth-sundar
force-pushed
the
146-update-python-dependencies
branch
from
8708899 to
a26c821
Compare
Closes #146
Just updated packages to be compatible with Python 3.8 and resolved MyPy/pylint complaints