Empty only #118 #122
Empty only #118 #122
Conversation
|
Thanks to @TeranIvy for volunteering to review. |
There was a problem hiding this comment.
Nicely done. I only have a couple of small remarks.
- Tests pass.
- Documentation builds (PDF and HTML).
pyecodestyleon modified files is OK.pylintonFortran2003.pyreports 2 warnings and it would be good to update one comment as well (see inline comments).- All lines in
class Use_Stmtare covered by tests. The tests are moved to a separate file with full support for R1109. - x-failing tests are not affected by the current issue.
- No change in the documentation is required as this is a bug fix.
src/fparser/two/Fortran2003.py
Outdated
| "Use_Stmt.tostr(). 'Items' should be of size 5 but found " | ||
| "'{0}'.".format(len(self.items))) | ||
| if not self.items[2]: | ||
| raise InternalError("Use_Stmt.tostr(). 'Items' entry 2 should " |
There was a problem hiding this comment.
pylint says W:7343, 0: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
There was a problem hiding this comment.
Oops, I missed that. Thanks.
src/fparser/two/Fortran2003.py
Outdated
| raise InternalError("Use_Stmt.tostr(). 'Items' entry 2 should " | ||
| "be a module name but it is empty") | ||
| if self.items[3] is None: | ||
| raise InternalError("Use_Stmt.tostr(). 'Items' entry 3 should " |
There was a problem hiding this comment.
pylint says W:7346, 0: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
| # Missing 'ONLY' specification after 'USE Module_Name,' | ||
| if not line: | ||
| return | ||
| if line[:4].upper() == 'ONLY': | ||
| line = line[4:].lstrip() | ||
| if not line: | ||
| # Expected ':' but there is nothing after the 'ONLY' |
There was a problem hiding this comment.
This comment describes nicely what the if test does. It is quite similar to the comment in line 7318 which is not the best description of what the if test in 7319 does (tests that character after ONLY is not the permitted :).
Would you kindly update the comment in 7318? I am aware the original developer made it unclear :-)
There was a problem hiding this comment.
I've hopefully made it more clear :-)
TeranIvy
added
reviewed with actions
|
I've hopefully addressed the reviewers comments. Back to @TeranIvy for second review. |
Codecov Report
@@ Coverage Diff @@
## master #122 +/- ##
==========================================
+ Coverage 88.13% 88.13% +<.01%
==========================================
Files 31 31
Lines 11264 11273 +9
==========================================
+ Hits 9927 9936 +9
Misses 1337 1337
Continue to review full report at Codecov.
|
rupertford
added
ready for review
and removed
reviewed with actions
|
Looks good and |
I've made the changes (which were small) and moved the associated tests into the fortran2003 directory, tidied etc. I think we're ready for first review.