Skip to content

Conversation

arporter
Copy link
Member

Small code fix and added new test for the associated routine.

@arporter arporter self-assigned this Jun 27, 2018
@arporter arporter requested review from rupertford and TeranIvy June 27, 2018 10:45
@arporter arporter changed the title For #81 - fix underfined variable in get_type_by_name() For #81 - fix undefined variable in get_type_by_name() Jun 27, 2018
@TeranIvy
Copy link
Collaborator

TeranIvy commented Jul 2, 2018

Happy to review it.

Copy link
Collaborator

@TeranIvy TeranIvy left a comment

Choose a reason for hiding this comment

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

The bug fix looks good. Please see the inline comments and pylint report below for the proposed changes.

  • All tests pass.
  • x-failing tests are unaffected by the current issue.
  • Modified/added tests do not use nose.
  • pycodestyle on added/modified code passes.
  • pylint on test_block_stmts.py is 10/10.
  • The interface to get_type_by_name(self, name) in modified class HasImplicitStmt(object) is now documented.
  • pylint returns some warnings and errors for the modified class (below). I am aware not all of them are for the modified lines, however I think would be good to improve the code quality for the entire class.
C: 99, 0: Missing class docstring (missing-docstring)
C:101, 4: Invalid class attribute name "a" (invalid-name)
E:129,56: Instance of 'HasImplicitStmt' has no 'item' member (no-member)
E:132,53: Instance of 'HasImplicitStmt' has no 'item' member (no-member)
C:135, 4: Missing method docstring (missing-docstring)
C:140,12: Invalid variable name "c" (invalid-name)
C:140,15: Invalid variable name "t" (invalid-name)
C:143,12: Invalid variable name "st" (invalid-name)
C:150, 8: Invalid variable name "s" (invalid-name)
C:151, 8: Invalid variable name "ls" (invalid-name)
C:152,12: Invalid variable name "st" (invalid-name)
C:152,16: Invalid variable name "l" (invalid-name)
C:155, 8: Invalid variable name "s" (invalid-name)
  • All modified/new code is covered, but not all of the existing code in get_type_by_name(self, name) (see inline comment).
  • Documentation builds correctly and it is up-to-date.
  • Please bring the branch up-to-date before the next review if required.

Returns an object of the correct type (Integer or Real) using
Fortran's implicit typing rules for the supplied variable name.
:param str name: the variable name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps The variable name as other descriptions are capitalised after :?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, done.

'''
implicit_rules = self.a.implicit_rules
if implicit_rules is None:
raise AnalyzeError('Implicit rules mapping is null '
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please consider adding tests to cover lines 116 and 120 so the entire function is covered. I am aware the missing coverage is not the result of this PR so this is up to the developer's good will :)

@TeranIvy TeranIvy added reviewed with actions PR has been reviewed and is back with developer and removed under review labels Jul 9, 2018
arporter added a commit that referenced this pull request Jul 9, 2018
@coveralls
Copy link

coveralls commented Jul 9, 2018

Coverage Status

Coverage increased (+0.4%) to 89.163% when pulling 7d50bfa on undev_var_fix into f5ad578 on master.

@codecov-io
Copy link

codecov-io commented Jul 9, 2018

Codecov Report

Merging #88 into master will increase coverage by 0.72%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
+ Coverage   88.73%   89.46%   +0.72%     
==========================================
  Files          23       24       +1     
  Lines       10513    10575      +62     
==========================================
+ Hits         9329     9461     +132     
+ Misses       1184     1114      -70
Impacted Files Coverage Δ
src/fparser/one/typedecl_statements.py 85.71% <100%> (+1.61%) ⬆️
src/fparser/one/tests/test_block_stmts.py 100% <100%> (ø)
src/fparser/one/block_statements.py 77.68% <100%> (+7.55%) ⬆️

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 f5ad578...7d50bfa. Read the comment docs.

arporter added a commit that referenced this pull request Jul 9, 2018
@arporter
Copy link
Member Author

arporter commented Jul 9, 2018

I've updated for pylint as requested and added further tests to hit the missed lines. They then revealed a Python 3 problem which I have also fixed.

@arporter
Copy link
Member Author

arporter commented Jul 9, 2018

I've rebased this branch onto the head of master. Ready for second review.

@arporter arporter added ready for review and removed reviewed with actions PR has been reviewed and is back with developer labels Jul 9, 2018
@arporter
Copy link
Member Author

arporter commented Jul 9, 2018

(Forgot to say that I haven't updated the class attribute named "a" because, even though it's a rubbish name, it permeates a lot of the code. Something for a separate issue.)

Copy link
Collaborator

@TeranIvy TeranIvy left a comment

Choose a reason for hiding this comment

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

Thank you for pylint clean-up, updating tests and fixing Python 3 issue in typedecl_statements.py. The last revealed some pycodestyle cleanup is required there, but that can be left for another day.
Proceeding to merge.

@TeranIvy TeranIvy merged commit f28a900 into master Jul 9, 2018
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.

4 participants