Skip to content
This repository was archived by the owner on Nov 17, 2025. It is now read-only.

Conversation

@koubaa
Copy link
Collaborator

@koubaa koubaa commented May 1, 2023

I also have to handle migrations (so that a user's configuration that points to ansys.mapdl.core or ansys.mechanical.core in appdirs is not discarded.

Also missing quite a few unit tests

@koubaa koubaa requested a review from germa89 May 1, 2023 15:36
Copy link
Contributor

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

LGTM.

We shall see how these changes affect also PyMAPDL since it has already implemented ansys-tools-path. Before finally merging this, I will check locally. Then I will finally approve.

So please, let me know when this is ready to merge and I will check locally.

==EDIT==

My previous comment makes me wonder if we should implement something on the CICD which tests against PyMAPDL. mmmhh.. Maybe not, because it should be PyMAPDL who adapts to this package and not the opposite.

@germa89 germa89 self-requested a review May 3, 2023 15:40
Copy link
Contributor

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

I checked locally and with my new commit we should be fine.

But many of the tests fail on a machine that does not have ansys installed. I wonder if we should have a flag to skip those.

By the way, I guess these changes will trigger a 0.2.0 release.

@koubaa
Copy link
Collaborator Author

koubaa commented May 3, 2023

I checked locally and with my new commit we should be fine.

But many of the tests fail on a machine that does not have ansys installed. I wonder if we should have a flag to skip those.

By the way, I guess these changes will trigger a 0.2.0 release.

That's a pre-existing issue - why don't you file an issue about that?

@koubaa
Copy link
Collaborator Author

koubaa commented May 3, 2023

@germa89 do you know why the test is failing in the CI/CD? I don't have a linux machine handy to try it

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #27 (d349020) into main (74e83b4) will decrease coverage by 15.54%.
The diff coverage is 67.47%.

@@             Coverage Diff             @@
##             main      #27       +/-   ##
===========================================
- Coverage   85.45%   69.91%   -15.54%     
===========================================
  Files           3        3               
  Lines         110      246      +136     
===========================================
+ Hits           94      172       +78     
- Misses         16       74       +58     

@germa89
Copy link
Contributor

germa89 commented May 3, 2023

@germa89 do you know why the test is failing in the CI/CD? I don't have a linux machine handy to try it

It is fixed now. The issue is you were trying to get a version which is not installed.

8b13e57

Copy link
Contributor

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

Added some pragma no cover. We should increase the coverage.

koubaa and others added 4 commits May 3, 2023 11:09
Co-authored-by: German <28149841+germa89@users.noreply.github.com>
Co-authored-by: German <28149841+germa89@users.noreply.github.com>
Co-authored-by: German <28149841+germa89@users.noreply.github.com>
Co-authored-by: German <28149841+germa89@users.noreply.github.com>
Co-authored-by: German <28149841+germa89@users.noreply.github.com>
@koubaa
Copy link
Collaborator Author

koubaa commented May 3, 2023

Added some pragma no cover. We should increase the coverage.

Yes, we should have an issue for that too

@koubaa koubaa merged commit 9aff519 into main May 3, 2023
@koubaa koubaa deleted the support-mechanical branch May 3, 2023 16:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants