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: tech review #53

Merged
merged 9 commits into from
Jul 21, 2023
Merged

MAINT: tech review #53

merged 9 commits into from
Jul 21, 2023

Conversation

RobPasMue
Copy link
Member

@RobPasMue RobPasMue commented Jul 21, 2023

Great job here @jorgepiloto and @Revathyvenugopal162 - green light from my side. Let me summarize my changes here:

Summary

  • Updating pre-commit and adding new hooks
  • Removing Python 3.7 from tox file
  • Specifying coverage report for ansys.tools.meilisearech and not ansys.tools (there are many other ansys.tools.* packages
  • Fix broken link on README
  • Avoid duplicated checkout in CI/CD

Question/Suggestion

  • Looking at your tests, I can see that you are "assuming" that the user has a local Meilisearch instance deployed on localhost at port 7700. I'd consider to include this in the documentation as a must-do, so that if anybody wants to contribute knows how to do it.
  • Also, following the legal reqs all source code files should have the Copyright headr. No big deal for now, let's wait for the hook to be ready. I wouldn't lose time on this for now. But remember adding the hook once this one goes public.

@RobPasMue RobPasMue self-assigned this Jul 21, 2023
@github-actions github-actions bot added documentation Improvements or additions to documentation maintenance Package and maintenance related enhancement New features or code improvements labels Jul 21, 2023
@RobPasMue
Copy link
Member Author

  • Looking at your tests, I can see that you are "assuming" that the user has a local Meilisearch instance deployed on localhost at port 7700. I'd consider to include this in the documentation as a must-do, so that if anybody wants to contribute knows how to do it.

Even for this last point... I'd consider adding docker as a dependency and launching the container through a fixture, in case the port is free. Might be a bit of an overkill but worth considering. Once tests finish, you can close the container. That'd make the whole process very "atomic"

@Revathyvenugopal162
Copy link
Collaborator

  • Looking at your tests, I can see that you are "assuming" that the user has a local Meilisearch instance deployed on localhost at port 7700. I'd consider to include this in the documentation as a must-do, so that if anybody wants to contribute knows how to do it.

Even for this last point... I'd consider adding docker as a dependency and launching the container through a fixture, in case the port is free. Might be a bit of an overkill but worth considering. Once tests finish, you can close the container. That'd make the whole process very "atomic"

Thanks for the suggestion Roberto, i will adapt this.

Copy link
Collaborator

@Revathyvenugopal162 Revathyvenugopal162 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 so much for the review and feedback @RobPasMue

@RobPasMue RobPasMue merged commit ea95e93 into main Jul 21, 2023
22 checks passed
@RobPasMue RobPasMue deleted the feat/tech-review branch July 21, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New features or code improvements maintenance Package and maintenance related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants