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

Approaches for Atbash Cipher #3457

Merged
merged 13 commits into from
Jul 31, 2023
Merged

Conversation

safwansamsudeen
Copy link
Contributor

No description provided.

Copy link
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

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

Hi @safwansamsudeen - Thanks for submitting this! Overall, it reads pretty well, but I am going to need to you change all the single letter variable names to something more readable in your code examples -- our track style is no single letter variable names - not even in comprehensions. Thanks for understanding. 🙂

@safwansamsudeen
Copy link
Contributor Author

Hey sorry for the delay. Is i for index in loops allowed?

@BethanyG
Copy link
Member

Hey sorry for the delay. Is i for index in loops allowed?

Nope. We are super-duper pendantic/conservative about it. index, item, member, etc.

@BethanyG
Copy link
Member

CI is broken at the moment. Trying to get it fixed without having to regenerate all the test files again, but I might have to anyways.

@safwansamsudeen
Copy link
Contributor Author

Nope. We are super-duper pendantic/conservative about it.
As you wish ;).

I've also added a link to a Programiz article explaining maketrans, based on a student request.

@safwansamsudeen
Copy link
Contributor Author

@BethanyG could we merge?

Copy link
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

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

There's a couple of places where the variable names were single letters. Beyond that, looks good.

Committing suggestions so the PR can be merged.
@BethanyG BethanyG self-requested a review July 31, 2023 15:12
@BethanyG BethanyG merged commit e3edaf8 into exercism:main Jul 31, 2023
8 checks passed
@safwansamsudeen safwansamsudeen deleted the atbash-approach branch August 30, 2023 00:33
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