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

Add solution to project_euler/problem_060 #8678

Closed
wants to merge 16 commits into from

Conversation

ignacio-gn
Copy link

Describe your change:

Add a solution to problem 60 of project Euler

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Apr 22, 2023
@algorithms-keeper algorithms-keeper bot added require descriptive names This PR needs descriptive function and/or variable names require type hints https://docs.python.org/3/library/typing.html labels Apr 22, 2023
Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

COUNTER = False


def get_order(n: int) -> int:

Choose a reason for hiding this comment

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

Please provide descriptive name for the parameter: n

return True


def is_prime(n) -> bool:

Choose a reason for hiding this comment

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

Please provide descriptive name for the parameter: n

Please provide type hint for the parameter: n

@algorithms-keeper algorithms-keeper bot added the awaiting reviews This PR is ready to be reviewed label Apr 22, 2023
@algorithms-keeper algorithms-keeper bot removed require descriptive names This PR needs descriptive function and/or variable names require type hints https://docs.python.org/3/library/typing.html labels Apr 22, 2023
@rohan472000
Copy link
Contributor

@ignacio-gn use ruff . before making PR, if fix needed then use ruff . --fix ,so that it will automatically fix it.

@ignacio-gn
Copy link
Author

@ignacio-gn use ruff . before making PR, if fix needed then use ruff . --fix ,so that it will automatically fix it.

I noticed that ruff fails for files that were already present in the repository, which I did not edit. Because of that, I decided to leave these files intact. I can attempt to fix them or rebase once they are fixed. Below is the error message:

Error: ciphers/rsa_cipher.py:79:13: UP031 Use format specifiers instead of percent format
[5](https://github.com/TheAlgorithms/Python/actions/runs/4773277000/jobs/8486249952?pr=8678#step:4:6)
Error: ciphers/rsa_cipher.py:104:13: UP031 Use format specifiers instead of percent format
[6](https://github.com/TheAlgorithms/Python/actions/runs/4773277000/jobs/8486249952?pr=8678#step:4:7)
Error: machine_learning/linear_discriminant_analysis.py:402:16: S605 Starting a process with a shell, possible injection detected
[7](https://github.com/TheAlgorithms/Python/actions/runs/4773277000/jobs/8486249952?pr=8678#step:4:8)
Error: Process completed with exit code 1.

@rohan472000
Copy link
Contributor

rohan472000 commented Apr 22, 2023

The first two errors suggest that we should use format specifiers instead of percent format. So we'll need to update the code in ciphers/rsa_cipher.pyon lines 79 and 104 to use format specifiers. For example, instead of using %d, we would use {} and then use .format() to fill in the value.

The third error suggests that there may be a possible injection detected. we should check the code in machine_learning/linear_discriminant_analysis.py on line 402 and ensure that any commands being executed are properly sanitized and do not pose any security risks.

Once we have made the necessary changes, you can run ruff . --fix to automatically fix any remaining issues. After that, you can rebase your changes and submit your updated pull request for review.

@rohan472000
Copy link
Contributor

rohan472000 commented Apr 22, 2023

@ignacio-gn I have made PR #8680 and #8679 to address above issue for both files, can you merge that two PR in one, rohan472000:patch-8 into rohan472000:patch-9 because I cant see both branches in my local after fork.

@rohan472000
Copy link
Contributor

I have merged both PR into one #8680

@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Apr 23, 2023
@rohan472000
Copy link
Contributor

commenting out that line 402 is working for now, which is good, but what alternative can we put there instead of that line???

@ignacio-gn
Copy link
Author

I have commented out the system call to clear the console on machine_learning/linear_discriminant_analysis.py. The reason for this is that I don't consider it to be relevant enough in the context of the program to implement an alternative that does not pose a security risk. The original call was there to clear the console on a new iteration of the program, but that behavior is not essential and it's blocking other PRs too (see 8684 and 8674).

I haven't contributed to machine_learning/linear_discriminant_analysis.py so I'd understand if you want to find another solution and to discard this one. In that case, let me know and I'll rollback the changes.

Copy link
Contributor

@rohan472000 rohan472000 left a comment

Choose a reason for hiding this comment

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

as #8680 is merged so there is no need to make any changes in those files, just run github actions again , it should work fine.

…ms#8680)

* Update rsa_cipher.py by replacing %s with {}

* Update rsa_cipher.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update linear_discriminant_analysis.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update linear_discriminant_analysis.py

* Update linear_discriminant_analysis.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update linear_discriminant_analysis.py

* Update linear_discriminant_analysis.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update linear_discriminant_analysis.py

* Update machine_learning/linear_discriminant_analysis.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update linear_discriminant_analysis.py

* updated

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Christian Clauss <cclauss@me.com>
@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Apr 24, 2023
@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Apr 24, 2023
@cclauss cclauss closed this Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting reviews This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants