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 adfgvx_cipher.py #3841

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@oops-creator
Copy link

@oops-creator oops-creator commented Oct 30, 2020

Describe your change:

  • 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 have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.
@oops-creator
Copy link
Author

@oops-creator oops-creator commented Oct 30, 2020

Some checks are failing. The problems seems to be incompatibility between flake8 and black, namely flake8 is not allowing whitespace before ':' but black adds the white space whenever I run the corresponding command. Can anyone please guide me on how to fix the conflict.

@mrmaxguns
Copy link
Contributor

@mrmaxguns mrmaxguns commented Nov 4, 2020

That is strange. What are the commands that you use for flake8 and black?

@oops-creator
Copy link
Author

@oops-creator oops-creator commented Nov 4, 2020

I am using flake8 as linter in vscode and running
flake8 adfgvx_cipher.py --ignore=E203,W503 --max-line-length=88 --show-source
and
python -m black adfgvx_cipher.py

@mrmaxguns
Copy link
Contributor

@mrmaxguns mrmaxguns commented Nov 5, 2020

I tried to reproduce this issue. By running black and then flake8, (the commands you specified above) both commands exited with 0, so everything is compatible. I have no idea why the checks are failing (probably the --ignore=E203,W503 flag was not specified for pre-commit).

@mrmaxguns
Copy link
Contributor

@mrmaxguns mrmaxguns commented Nov 5, 2020

This looks like an issue with the pre-commit system itself.

@mrmaxguns
Copy link
Contributor

@mrmaxguns mrmaxguns commented Nov 5, 2020

Try to reproduce the issue locally by running black, then pep8, and pre-commit run --all-files. (make sure you have pre-commit installed with pip install pre-commit)

@oops-creator
Copy link
Author

@oops-creator oops-creator commented Nov 5, 2020

I dont know what the issue was but I reinstalled flake8 and black and it seems to be working now. I will make a new pull request now. Thanks for the help. Hope I didnt waste your time.

Copy link
Contributor

@mrmaxguns mrmaxguns left a comment

In this case, exceptions are preferred over sys.exit due to the fact that exceptions provide a traceback (the user of the algorithm will be able to see where the problem occurred) and can also be easily handled by the user in try, except blocks.

ciphers/adfgvx_cipher.py Outdated Show resolved Hide resolved
ciphers/adfgvx_cipher.py Outdated Show resolved Hide resolved
ciphers/adfgvx_cipher.py Outdated Show resolved Hide resolved
@oops-creator oops-creator force-pushed the oops-creator:master branch from b19cae7 to 4733126 Nov 6, 2020
@oops-creator
Copy link
Author

@oops-creator oops-creator commented Nov 6, 2020

Thanks for the input. I have done the suggested edits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.