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 atbash-cipher #274

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Add atbash-cipher #274

merged 3 commits into from
Sep 12, 2024

Conversation

BNAndras
Copy link
Member

@BNAndras BNAndras commented Sep 11, 2024

Closes #164

@0xNeshi 0xNeshi added the x:rep/large Large amount of reputation label Sep 11, 2024
Copy link
Contributor

@0xNeshi 0xNeshi left a comment

Choose a reason for hiding this comment

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

We can merge this despite the results of the test-cases-related topic https://forum.exercism.org/t/atbash-cipher-test-cases-might-be-wrong/12959

If there comes a need to change the tests, will create a separate issue

It is a very weak cipher because it only has one possible key, and it is a simple mono-alphabetic substitution cipher.
However, this may not have been an issue in the cipher's time.

Ciphertext is written out in groups of fixed length, the traditional group size being 5 letters, leaving numbers unchanged, and punctuation is excluded.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very weird definition of atbash cipher; it generally just ignores punctuation and doesn't group letters in any way.
I opened a topic on the official forum about this https://forum.exercism.org/t/atbash-cipher-test-cases-might-be-wrong/12959

Copy link
Member Author

Choose a reason for hiding this comment

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

If you're proposing a change to the canonical test suite, I'd be mindful that that problem spec has been around for ten years and has been implemented across 60+ tracks. It'd take some time and work to first sync the changes to the tracks, and then we'll need to rerun all the completed solutions. All solutions would fail if we're no longer grouping strings in five letter chunks. So you'll get some pushback for sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not that we can't change the test suite, but we should have a good reason like there's an edge case to be tested that might be significant for multiple tracks.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was just seemed confusing, because I know what the actual atbash cipher requires. That's why I opened a topic on the forum to discuss it, but this won't affect this PR, which will be reviewed with the current problem description in mind.

exercises/practice/atbash-cipher/.meta/example.cairo Outdated Show resolved Hide resolved
exercises/practice/atbash-cipher/.meta/example.cairo Outdated Show resolved Hide resolved
exercises/practice/atbash-cipher/.meta/example.cairo Outdated Show resolved Hide resolved
@BNAndras
Copy link
Member Author

All tests pass locally and in the CI but it's reporting a failure.

@0xNeshi
Copy link
Contributor

0xNeshi commented Sep 12, 2024

That's weird 🤔 Investigating

@0xNeshi
Copy link
Contributor

0xNeshi commented Sep 12, 2024

@BNAndras the issue was in the exit 1 after the timeout of cairo tests - it ran no matter the result of the test (success/fail). Fixed this by simply changing the order of precedence of commands so that exit 1 runs only if tests fail.
Merge #280 ASAP and we can merge the PR too

@0xNeshi 0xNeshi merged commit a542481 into exercism:main Sep 12, 2024
4 of 5 checks passed
@BNAndras BNAndras deleted the add-atbash-cipher branch September 12, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:rep/large Large amount of reputation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Practice Exercise: atbash-cipher
2 participants