Skip to content

Remove some Py2-specific code - #981

Merged
dwhswenson merged 6 commits into
openpathsampling:dev-2.0from
dwhswenson:drop-py2
Feb 23, 2021
Merged

Remove some Py2-specific code#981
dwhswenson merged 6 commits into
openpathsampling:dev-2.0from
dwhswenson:drop-py2

Conversation

@dwhswenson

Copy link
Copy Markdown
Member

#977 dropped Python 2 from the OPS 2.0 test matrix. In the process, some code lost coverage. Here I remove the Py2-specific code for most of that (except netcdfplus, which I'm not worrying about since it will also be dropped in 2.0).

There's one line where I don't immediately understand how it lost coverage:

okay_options[variable] = my_options[variable]

This is in an instance test against basestring, but basestring is str for Py3+ (we set it at the top of the file). Any idea why that's not getting covered? (particularly @sroet)

Note that this PR is not intended to drop all Py2-related code -- there's more to do on that. This is just catching the things that lost coverage when Py2 left the test matrix.

@dwhswenson dwhswenson added 2.0 issues and PRs for the 2.0 release drop-py2 labels Feb 21, 2021
@sroet

sroet commented Feb 21, 2021

Copy link
Copy Markdown
Member

Looking at the code, shouldn't that always (assuming str) be cought by line 225 into 251?

225                     if type(my_options[variable]) is type(default_value):

251                        else:
                                okay_options[variable] = my_options[variable]

What input type do you expect to be a subclass of str without being a str?

@codecov

codecov Bot commented Feb 22, 2021

Copy link
Copy Markdown

Codecov Report

Merging #981 (82d5948) into dev-2.0 (5691722) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           dev-2.0     #981   +/-   ##
========================================
  Coverage    80.25%   80.26%           
========================================
  Files          136      136           
  Lines        14449    14455    +6     
========================================
+ Hits         11596    11602    +6     
  Misses        2853     2853           
Impacted Files Coverage Δ
openpathsampling/ensemble.py 84.59% <100.00%> (+0.08%) ⬆️
openpathsampling/netcdfplus/netcdfplus.py 75.00% <100.00%> (ø)
openpathsampling/netcdfplus/version.py 100.00% <100.00%> (ø)
openpathsampling/numerics/histogram.py 83.49% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ff2cbb...b3365df. Read the comment docs.

@dwhswenson

Copy link
Copy Markdown
Member Author

What input type do you expect to be a subclass of str without being a str?

Yeah, that's it. Py 2.7 had str and unicode that inherited from basestring.

I removed that line and all references to basestring from that file.

@dwhswenson

Copy link
Copy Markdown
Member Author

This is ready for review and comment. I will leave it open for at least 24 hours, merging no earlier than Tue 23 Feb 14:00 GMT (15:00 local).

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

This looks good to me. For any other reviewer: the changes in ensemble.py are identical to the #976

@dwhswenson
dwhswenson merged commit e0d2076 into openpathsampling:dev-2.0 Feb 23, 2021
@dwhswenson
dwhswenson deleted the drop-py2 branch February 23, 2021 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.0 issues and PRs for the 2.0 release drop-py2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants