Skip to content

Remove last traces of python 2 special handling#328

Merged
arporter merged 23 commits intomasterfrom
more-remove-six
May 19, 2022
Merged

Remove last traces of python 2 special handling#328
arporter merged 23 commits intomasterfrom
more-remove-six

Conversation

@ZedThree
Copy link

@ZedThree ZedThree commented Apr 6, 2022

PR into #322 removing last few bits of six and python 2 special handling

Base automatically changed from 307_byebye_py2 to master May 9, 2022 16:58
@arporter
Copy link
Member

Hi @ZedThree, if you could fix the conflicts on this one and then I'll review it... Thanks :-)

* master:
  pr #322. Updated changelog ready for merge to master.
  #322 try quoting python versions in GHA yml
  #322 updates for review
  #323 update changelog
  Use Sphinx roles to some docstrings
  Add some test docstrings
  Move entity decl test under f2003 directory
  Auto-use parser creation fixtures
  Add more tests for `get_name` methods
  Add `.get_name()` to `Function_Stmt` and `Entity_Decl`
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #328 (1756d9b) into master (27628aa) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
- Coverage   91.28%   91.25%   -0.04%     
==========================================
  Files          36       36              
  Lines       13046    13022      -24     
==========================================
- Hits        11909    11883      -26     
- Misses       1137     1139       +2     
Impacted Files Coverage Δ
src/fparser/common/tests/test_sourceinfo.py 96.74% <ø> (-0.03%) ⬇️
src/fparser/one/tests/test_block_stmts.py 100.00% <ø> (ø)
src/fparser/one/tests/test_do_block_r814.py 100.00% <ø> (ø)
src/fparser/one/tests/test_scripts.py 100.00% <ø> (ø)
src/fparser/scripts/parse.py 79.41% <ø> (-0.59%) ⬇️
src/fparser/scripts/read.py 96.00% <ø> (-0.16%) ⬇️
src/fparser/api.py 80.00% <100.00%> (-0.31%) ⬇️
src/fparser/common/base_classes.py 69.03% <100.00%> (-0.15%) ⬇️
src/fparser/common/readfortran.py 94.59% <100.00%> (-0.16%) ⬇️
src/fparser/common/sourceinfo.py 100.00% <100.00%> (ø)
... and 18 more

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 27628aa...1756d9b. Read the comment docs.

@ZedThree
Copy link
Author

@arporter Good to go :)

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #328 (be2fd65) into master (27628aa) will decrease coverage by 0.01%.
The diff coverage is 89.23%.

@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
- Coverage   91.28%   91.27%   -0.02%     
==========================================
  Files          36       36              
  Lines       13046    12994      -52     
==========================================
- Hits        11909    11860      -49     
+ Misses       1137     1134       -3     
Impacted Files Coverage Δ
src/fparser/common/tests/test_sourceinfo.py 96.74% <ø> (-0.03%) ⬇️
src/fparser/one/tests/test_block_stmts.py 100.00% <ø> (ø)
src/fparser/one/tests/test_do_block_r814.py 100.00% <ø> (ø)
src/fparser/one/tests/test_scripts.py 100.00% <ø> (ø)
src/fparser/scripts/parse.py 79.41% <ø> (-0.59%) ⬇️
src/fparser/scripts/read.py 96.00% <ø> (-0.16%) ⬇️
src/fparser/two/Fortran2003.py 93.50% <36.36%> (-0.01%) ⬇️
src/fparser/api.py 80.00% <100.00%> (-0.31%) ⬇️
src/fparser/common/base_classes.py 68.33% <100.00%> (-0.85%) ⬇️
src/fparser/common/readfortran.py 94.81% <100.00%> (+0.07%) ⬆️
... and 14 more

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 27628aa...be2fd65. Read the comment docs.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for taking this on Peter - it's a big improvement.
GHA and coverage is all fine.
Documentation builds and looks fine.
There's one method that still talks quite a lot about Py2 and Py3.
There's also one test that you've removed completely and I'm not sure why.
Apart from that, there are just a few places where I've taken the opportunity to ask for fixes to associated pylint warnings. (I tried to keep this to a minimum because otherwise you'd be here all year given the state of some of the existing code.)

@arporter arporter added reviewed with actions PR has been reviewed and is back with developer and removed under review labels May 10, 2022
@ZedThree
Copy link
Author

One other change that I've not done but just seen is use of match = staticmethod(match) which could be converted to be a decorator on the method instead. This is a pretty mechanical fix (just taken me a couple of seconds to do with an Emacs macro), would you like that in here too? That's 135 lines

@arporter
Copy link
Member

One other change that I've not done but just seen is use of match = staticmethod(match) which could be converted to be a decorator on the method instead. This is a pretty mechanical fix (just taken me a couple of seconds to do with an Emacs macro), would you like that in here too? That's 135 lines

I think I'd like that kept for a separate PR (not necessarily by you!) - this is already quite big.

@arporter
Copy link
Member

Hi @ZedThree, is this ready for review again?

@ZedThree
Copy link
Author

@arporter Yep, all done!

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there apart from those places where a return None is needed :-)
Please note that our way of working on GitHub is for the reviewer to mark conversations as 'resolved' as that makes it easier for them to ensure that each has been done to their satisfaction.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for the updates.
Just a couple of comments to address and then this can be merged.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for another go now...

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good now thanks. Will proceed to merge once CI is complete (and happy).

@arporter arporter added ready for merge PR is waiting on final CI checks before being merged. and removed reviewed with actions PR has been reviewed and is back with developer labels May 19, 2022
@arporter
Copy link
Member

The missed lines are all statements where return has been replaced by return None in Fortran2003.py. These weren't covered previously and I'm not going to insist they be covered now.

@arporter arporter merged commit 8e8ae1d into master May 19, 2022
@arporter arporter deleted the more-remove-six branch May 19, 2022 15:40
ZedThree added a commit that referenced this pull request May 19, 2022
* master: (22 commits)
  #328 update changelog
  Rename `BeginStatement.handle_unknown_item`
  Return `None` from unimplemented function
  Use `f` prefix on all lines of multiline f-string
  Return `None` explicitly from some functions
  Remove redundant `return` statements from the end of functions
  Convert some `str.format()` calls to f-strings
  Remove py2 `unicode` string literal
  Remove py2 `unicode` special handling
  Change type in docstring to py3 style hint
  Remove reference to py2 from comments/docstring
  Revert accidental removal of test case
  Fix wrong variable in error message
  Remove duplicated `str` in `isinstance` argument
  Use f-string to print error message
  Remove mention of `six` from documentation
  Use py3 style class/metaclass syntax
  Remove imports of `__future__`
  Remove use of `six.StringIO`
  Remove some special handling for differences in capsys between py2/3
  ...
ZedThree added a commit to PlasmaFAIR/fparser that referenced this pull request Jun 9, 2022
* master: (44 commits)
  stfc#336 update changelog
  stfc#343 update version to 0.0.15
  Update changelog
  stfc#328 update changelog
  Rename `BeginStatement.handle_unknown_item`
  Return `None` from unimplemented function
  Use `f` prefix on all lines of multiline f-string
  Return `None` explicitly from some functions
  Remove redundant `return` statements from the end of functions
  Convert some `str.format()` calls to f-strings
  Remove py2 `unicode` string literal
  Remove py2 `unicode` special handling
  Change type in docstring to py3 style hint
  Remove reference to py2 from comments/docstring
  Revert accidental removal of test case
  Fix wrong variable in error message
  Remove duplicated `str` in `isinstance` argument
  pr stfc#322. Updated changelog ready for merge to master.
  stfc#322 try quoting python versions in GHA yml
  stfc#322 updates for review
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for merge PR is waiting on final CI checks before being merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants