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

Formatting/tests #5

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

VascoSch92
Copy link
Contributor

@VascoSch92 VascoSch92 commented Oct 4, 2023

Sorry, I closed the old PR an di opened a new one ;-)

Ps: the tests are failing not bevause of my code. But i will correct that ;) when i have time

@VascoSch92
Copy link
Contributor Author

There are errors because pyflake8 doesn't like when the symbol \ is in the doctoring of a method

@VascoSch92
Copy link
Contributor Author

@tommyod Hey 👋 what do you think?

@tommyod
Copy link
Owner

tommyod commented Mar 11, 2024

Hi! The logs have expired, the the tests did not pass. I am not sure you why. Could you push a new commit to re-trigger so we get logs and can figure out why tests fail? We need everything to pass before merging.

As for the PR itself I have not looked at it in detail yet. Will do it once tests pass! Here you wrote that you wanted to use pytest, which seems like a good idea. However, in the PR you also remove some docstrings, change latex notation (e.g. Z_n instead of \mathbb{Z}_n), etc. If you want to do this, you should (1) explain why and (2) do it in a separate PR.

Still happy to merge this, but (1) lets get tests to pass and (2) please remove anything not related to pytest from this PR (you can move the changes to those files a different branch in git). Keeping PRs related to one issue at a time makes them easier to review.

@VascoSch92
Copy link
Contributor Author

Hi! The logs have expired, the the tests did not pass. I am not sure you why. Could you push a new commit to re-trigger so we get logs and can figure out why tests fail? We need everything to pass before merging.

As for the PR itself I have not looked at it in detail yet. Will do it once tests pass! Here you wrote that you wanted to use pytest, which seems like a good idea. However, in the PR you also remove some docstrings, change latex notation (e.g. Z_n instead of \mathbb{Z}_n), etc. If you want to do this, you should (1) explain why and (2) do it in a separate PR.

Still happy to merge this, but (1) lets get tests to pass and (2) please remove anything not related to pytest from this PR (you can move the changes to those files a different branch in git). Keeping PRs related to one issue at a time makes them easier to ...

The problem with the docstring is described in the comment above. But perhaps is also something other. I wanted also to look at the logs ;)

I can trigger the tests with a new commit (i cannot re-run the workflows myself) and i will give a closer look. Then we can decide according.😉

@tommyod
Copy link
Owner

tommyod commented Mar 11, 2024

Sounds good! I found this and this when googling. See if it fixes the problem!

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.

2 participants