Skip to content

Fix bug: if construct has name, it must appear in end statement#325

Merged
arporter merged 9 commits intostfc:masterfrom
PlasmaFAIR:fix-construct-name-constraint
Jun 9, 2022
Merged

Fix bug: if construct has name, it must appear in end statement#325
arporter merged 9 commits intostfc:masterfrom
PlasmaFAIR:fix-construct-name-constraint

Conversation

@ZedThree
Copy link

@ZedThree ZedThree commented Apr 5, 2022

Affects following block constructs:

  • associate
  • do (non-label)
  • if
  • select case
  • select type
  • where

This more strictly implements the constraints on the various blocks, which all have wording similar to the do-construct:

C821 If the do-stmt of a block-do-construct specifies a do-construct-name, the corresponding
end-do shall be an end-do-stmt specifying the same do-construct-name. If the do-stmt of a
block-do-construct does not specify a do-construct-name, the corresponding end-do shall not
specify a do-construct-name.

Basically, if you use name: do, you must use end do name.

Affects following block constructs:
- `associate`
- `do` (non-label)
- `if`
- `select case`
- `select type`
- `where`
@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #325 (779182b) into master (982812c) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
+ Coverage   91.28%   91.33%   +0.05%     
==========================================
  Files          36       36              
  Lines       13010    13028      +18     
==========================================
+ Hits        11876    11899      +23     
+ Misses       1134     1129       -5     
Impacted Files Coverage Δ
src/fparser/two/Fortran2003.py 93.63% <100.00%> (+0.12%) ⬆️
src/fparser/two/utils.py 95.47% <100.00%> (+0.01%) ⬆️

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 982812c...779182b. Read the comment docs.

@arporter
Copy link
Member

Hi @ZedThree, just getting back up to speed with things. I just have a question about why we need a 'strict' option in addition to the match_names option. Once I understand that, I'll do a proper review (although I can warn you ahead of time that there are a lot of docstrings missing :-) ).

@arporter arporter added reviewed with actions PR has been reviewed and is back with developer and removed under review labels Apr 19, 2022
@arporter arporter added under review and removed reviewed with actions PR has been reviewed and is back with developer labels May 24, 2022
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 fixing this and adding all of the tests. In general I think it's good but the tests need docstrings and some additional checks to ensure that the error being raised is the one we expect.

@arporter arporter added reviewed with actions PR has been reviewed and is back with developer and removed under review labels May 25, 2022
ZedThree added 8 commits June 9, 2022 09:36
`else`/`case` sub-blocks are allowed to have missing names
* 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
  ...
@ZedThree
Copy link
Author

ZedThree commented Jun 9, 2022

Changes made. Adding the more explicit checks for the exception messages caught a bug: else/case statements aren't required to be named.

Also merged in master to fix conflicts

@arporter arporter added under review and removed reviewed with actions PR has been reviewed and is back with developer labels Jun 9, 2022
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.

Thanks for making all those changes.
I've taken the liberty of fixing various lines that were too long (although I now find that there are a lot of those that are nothing to do with this PR).
I'll proceed to merge.

@arporter arporter merged commit 60a2ec0 into stfc:master Jun 9, 2022
arporter added a commit that referenced this pull request Jun 9, 2022
@arporter
Copy link
Member

arporter commented Jun 9, 2022

Actually, since I couldn't update the upstream branch (because it's in PlasmaFAIR), I merged it as is and created #346 to do the pycodestyle tidying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants