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

New version overlap.py to fix_factorial2 #309 with unittest named test_factorial2 #316

Closed
wants to merge 5 commits into from

Conversation

D-TheProgrammer
Copy link
Contributor

@LudoRNLT and I think that we have solved the fix for issue #309 described in issue #308 for this pull request. As suggested, we have added a unit test, and our code is passing both the test_factorial2 and test_overlap unit tests. Please let us know if this is good and if it will be merged. Thank you.
Capture d’écran du 2024-05-24 21-50-39

Capture d’écran du 2024-05-24 21-54-10

D-TheProgrammer and others added 3 commits May 24, 2024 21:33
@tovrstra
Copy link
Member

Thanks for the pull request. I'll review it and plan to merge it after #315 has been merged.

@tovrstra tovrstra self-requested a review May 25, 2024 09:00
Copy link
Member

@tovrstra tovrstra left a comment

Choose a reason for hiding this comment

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

Again, thank you for this contribution! I've added a few quick comments. I'll check in more detail before merging.

iodata/test/test_factorial2.py Outdated Show resolved Hide resolved
iodata/test/test_factorial2.py Outdated Show resolved Hide resolved
iodata/test/test_factorial2.py Outdated Show resolved Hide resolved
@tovrstra tovrstra linked an issue May 25, 2024 that may be closed by this pull request
@D-TheProgrammer
Copy link
Contributor Author

@tovrstra We have followed all your comments and changed the unittest to pytest_approx. We apologize for the mistake; we were basing our work on your previous comment on issue #308, which led us to believe we should use unittest. The test_factorial2 has been updated and posted in this pull request. If you don't see it, please let us know, and we will create a new pull request.

Thank you, and please let us know if you merge it.

@tovrstra
Copy link
Member

I just merged #315 and well merge your pull request into the current main branch to enable the Ruff linters on this pull request.

@tovrstra
Copy link
Member

It seems I cannot push the merge with the current main into your pull request. This may be related to the fact that you made a pull request from the main branch of your repository, but I'm not sure. (See branch protection rules in the GitHub documentation.) It is in general not recommended to develop on the main branch of a fork, because that makes it impossible to sync your fork with the original repository. You may have to remove and recreate your fork to fix this.

Could you create a new pull request starting from the current main? The current main has a series of new linters enabled, which prevent many anti-patterns from being committed.

When doing so, could you run the following in your local repository before making a commit?

pre-commit install

That way, you run the linters before committing to your git branch.

Thank you!

@D-TheProgrammer D-TheProgrammer closed this by deleting the head repository May 30, 2024
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.

Fix Factorial2
2 participants