Skip to content

Implement pyansys-tools-report in PyMAPDL #1215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Jun 28, 2022
Merged

Conversation

RobPasMue
Copy link
Member

Closes #1195

@RobPasMue
Copy link
Member Author

Not really understanding why its complaining... importlib.metadata should be available, since it is part of the pyproject.toml reqs...

@codecov
Copy link

codecov bot commented Jun 21, 2022

Codecov Report

Merging #1215 (8dd7a71) into main (adfbe6a) will increase coverage by 0.01%.
The diff coverage is 87.75%.

@@            Coverage Diff             @@
##             main    #1215      +/-   ##
==========================================
+ Coverage   78.02%   78.04%   +0.01%     
==========================================
  Files          43       43              
  Lines        6777     6777              
==========================================
+ Hits         5288     5289       +1     
+ Misses       1489     1488       -1     

@RobPasMue
Copy link
Member Author

Why is it complaining about coverage?? @akaszynski @germa89 I understand that you want 90% or more in your library... but as far as I can tell... my changes are covered. Is it requiring me to cover more of the PyMAPDL code?

@RobPasMue RobPasMue marked this pull request as ready for review June 21, 2022 10:57
@RobPasMue RobPasMue requested review from germa89 and akaszynski and removed request for germa89 and akaszynski June 21, 2022 10:58
@akaszynski
Copy link
Collaborator

Why is it complaining about coverage?? @akaszynski @germa89 I understand that you want 90% or more in your library... but as far as I can tell... my changes are covered. Is it requiring me to cover more of the PyMAPDL code?

The issue is that these lines were not originally covered. Let's cover this in a separate PR if needed, unless you want to add coverage for these lines.

@RobPasMue
Copy link
Member Author

Why is it complaining about coverage?? @akaszynski @germa89 I understand that you want 90% or more in your library... but as far as I can tell... my changes are covered. Is it requiring me to cover more of the PyMAPDL code?

The issue is that these lines were not originally covered. Let's cover this in a separate PR if needed, unless you want to add coverage for these lines.

Understood, no worries. I'll set up some more tests. Thanks @akaszynski for the explanation.

@RobPasMue
Copy link
Member Author

The remaining lines can not be easily tested:

  • One assumes there are no Ansys environment variables (which is not possible to overcome)
  • The other assumes there is a MAPDL installation locally (GH containers do not have MAPDL installed)
  • The last one (calling Report constructor and redirecting to Plain_Report) is not callable, since in the test requirements there is already a requirement for pyansys-tools-report, and thus it is not accessible.

I consequently propose to close this PR as is... Solving the coverage for those conditions would be too much work for the gain they suppose...

Pinging @akaszynski @germa89 for feedback

@RobPasMue
Copy link
Member Author

@germa89 can we try and close this one? =)

@germa89 germa89 merged commit 06f9091 into main Jun 28, 2022
@germa89 germa89 deleted the feat/pyansys-tools-report branch June 28, 2022 08:54
@germa89
Copy link
Collaborator

germa89 commented Jun 28, 2022

Merged without coverage check. It wasn't worthy. Thank you @RobPasMue !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make use of pyansys-tools-report
3 participants