Skip to content
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

maint: technical review #31

Merged
merged 70 commits into from
Feb 15, 2024
Merged

maint: technical review #31

merged 70 commits into from
Feb 15, 2024

Conversation

clatapie
Copy link
Contributor

@clatapie clatapie commented Aug 14, 2023

This pull-request performs the technical review of this project.

Changes by this PR:

  • maint: .gitignore updated
  • maint: pre-commit fixed
  • maint: pyproject.toml modified
  • maint: use flit instead of poetry as the building system
  • README.rst file updated
  • documentation updated
  • CICD updated
  • licensing headers added

To be done:

@clatapie clatapie self-assigned this Aug 14, 2023
@clatapie
Copy link
Contributor Author

The pyansys-tools-variableinterop needs to be released first as it is a dependency of this package.

@phx-mkoninckx
Copy link
Contributor

@clatapie : I'm not sure that increasing unit test coverage for this repo is a requirement we can meet. If you inspect the Python code in this repo in detail, you'll find that it is entirely abstract base classes -- there's no "executable" code here in some sense. The repo exists only to provide a place to store a common base for Python APIs of engineering workflow engines here at Ansys in general (i.e. a common base between ModelCenter and optiSLang). I'm not 100% certain of the function of the existing unit test or why it is named what it is but it seems to me that it achieves the goal of checking whether this module can be imported, which is really all that needs to happen to it. @phxnsharp may be able to provide more detail.

However, if there is a way that Ansys typically likes to test API-only python packages such as this one I'm sure we can implement that. Perhaps there's some information here I'm missing.

@phxnsharp
Copy link
Collaborator

I'm not 100% certain of the function of the existing unit test or why it is named what it is but it seems to me that it achieves the goal of checking whether this module can be imported, which is really all that needs to happen to it. @phxnsharp may be able to provide more detail.

I don't see the value in meeting some arbitrary code coverage target merely to mark off a checkbox. You are done testing when you have met the confidence level the business requires.

@clatapie
Copy link
Contributor Author

Thank you for your feedback. As you asked, I removed the requirement for the 80% test coverage as this repository only contains classes and abstract methods.
The only remaining task is then the documentation review. For this topic, I am pinging @PipKat for visibility. Depending on what she prefers, we might create a new PR to perform it.

@clatapie
Copy link
Contributor Author

The dependency error should be fixed once pyansys-tools-variableinterop will be released.

@PipKat
Copy link
Member

PipKat commented Oct 20, 2023

Thank you for your feedback. As you asked, I removed the requirement for the 80% test coverage as this repository only contains classes and abstract methods. The only remaining task is then the documentation review. For this topic, I am pinging @PipKat for visibility. Depending on what she prefers, we might create a new PR to perform it.

@clatapie I would prefer to create a PR for the doc review. If you would please ping me when this PR is merged, I'll start a doc review. Also, I don't see a link in the right column to access the documentation for this repo. Would you add it there or at least send me the URL? Thanks.

@clatapie
Copy link
Contributor Author

Thank you @PipKat, I will ping you once this PR will be merged.

@clatapie clatapie requested a review from RobPasMue February 15, 2024 11:41
RobPasMue
RobPasMue previously approved these changes Feb 15, 2024
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

.github/dependabot.yml Outdated Show resolved Hide resolved
.github/workflows/cicd.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
CONTRIBUTORS.md Outdated Show resolved Hide resolved
LICENSE Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@clatapie clatapie requested a review from RobPasMue February 15, 2024 13:02
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

LGTM - nice job @clatapie

@clatapie clatapie merged commit 2f65008 into main Feb 15, 2024
20 checks passed
@clatapie clatapie deleted the maint/tech_review branch February 15, 2024 13:59
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.

5 participants