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

Correct capitalization of 'atbash' to 'Atbash' #2485

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

tasxatzial
Copy link
Member

No description provided.

@tasxatzial tasxatzial requested a review from a team as a code owner October 22, 2024 21:02
@BNAndras
Copy link
Member

Although all monoalphabetic ciphers are weak, the affine cipher is much stronger than the atbash cipher, because it has many more keys.
includes another lowercased Atbash reference.

@BNAndras
Copy link
Member

Also, if it doesn't cause additional churn for tracks with test generators, the comments in atbash-cipher tests.toml also use the lowercased Atbash.

@tasxatzial
Copy link
Member Author

tasxatzial commented Oct 22, 2024

So far 'Atbash' is incorrectly capitalized in:

Although all monoalphabetic ciphers are weak, the affine cipher is much stronger than the atbash cipher, because it has many more keys.

blurb = "Create an implementation of the atbash cipher, an ancient encryption system created in the Middle East."

"* Encoding from English to atbash cipher",

"* Decoding from atbash cipher to all-lowercase-mashed-together English"

I'll update those too, if needed. I just need to know which ones.

Edit: I guess the affine-cipher instructions need to be fixed for sure.

@IsaacG
Copy link
Member

IsaacG commented Oct 23, 2024

I think it would be good to update them all to be consistent.

@tasxatzial tasxatzial changed the title atbash-cipher: Correct capitalization of 'atbash' to 'Atbash' in instructions Correct capitalization of 'atbash' to 'Atbash' Oct 23, 2024
@kotp
Copy link
Member

kotp commented Nov 20, 2024

  • [403] https://www.4clojure.com/ | Failed: Network error: Forbidden
    This is returning the 403 from the website itself, and so this fails correctly.

This test is correctly failing, but not for reasons of this PR.

@BNAndras
Copy link
Member

That was fixed in #2487 so we just need a rebase.

@tasxatzial
Copy link
Member Author

@BNAndras You are referring to this PR? It doesn't seem to be conflicting with the main branch. Only the link check fails, which doesn't affect anything.

@kotp
Copy link
Member

kotp commented Nov 20, 2024

@BNAndras You are referring to this PR? It doesn't seem to be conflicting with the main branch. Only the link check fails, which doesn't affect anything.

It is not that it is conflicting, it is that it out of date to what it is based on, not allowing the CI tests to pass. If you check out main, get that branch up to date locally, and then rebase this branch to that, and push it up, you will find that the updated changes are in place allowing the CI test to pass.

The commands that I would do would be something like this:

git checkout main
git pull upstream main
git checkout atbash-cipher
git rebase upstream//main
git push origin atbash-cipher

Provided, of course, that the remote for the exercism repository is upstream.

You may need to force push, which is fine, as this is your own branch, not the history on main on the "canonical repository".

To expand, in the web view on your own repository, you will see information such as:

"This branch is 2 commits ahead of, 8 commits behind exercism/problem-specifications:main."

This is what is causing the problem, there may be some kind of on platform "rebase this to the upstream remote" or something so that you may not need to do this in the command line, but I most often work there, so am more familiar with what needs to be done locally.

@iHiD

This comment was marked as off-topic.

@tasxatzial
Copy link
Member Author

I understand that the CI check links does not pass, what I don't understand why they need to pass?

For example, commits on 9 Oct, 22 Oct, 24 Oct, were all having the same issue with the failing links.

@IsaacG
Copy link
Member

IsaacG commented Nov 21, 2024

I understand that the CI check links does not pass, what I don't understand why they need to pass?

For example, commits on 9 Oct, 22 Oct, 24 Oct, were all having the same issue with the failing links.

There was a known issue with the check. People were discussing it and trying to figure out how to beat deal with it. While there was a known issue with the check, it was reasonable to ignore the failures.

The issue has since been fixed and the check should now be working. There is no longer any reason for the tests to be failing.

@Cool-Katt
Copy link
Contributor

@tasxatzial looks like your fork of the repo is a few commits behind so it didn't get the fux that Isaac was talking about. (It's relatively recent, at least the specific case that's making the CI fail here)

@tasxatzial
Copy link
Member Author

Ok, so no particular reason other than making the checks pass. When BNAndras said it was needed, I thought there was a more serious issue, like deployment failing or something similar, hence the question.

Rebased.

@BNAndras
Copy link
Member

Thanks. I was just clarifying what needed to be after KOTP highlighted the failing test. The PR itself can’t be merged without approval from the code owner.

@kotp
Copy link
Member

kotp commented Nov 21, 2024

git checkout atbash-cipher
git pull --rebase upstream main
git push origin atbash-cipher

Know that the shorter way will not update your main, though, but will only rebase the branch to upstream/main.

Probably not a problem if you are aware, but may cause confusion if you are not.

@kotp
Copy link
Member

kotp commented Nov 21, 2024

Also, thanks @tasxatzial for the rebase, and @BNAndras for the clarification, @iHiD for the lesson in git efficiency, and @Cool-Katt for reiteratig and @IsaacG for the time travel exploration!

@BNAndras
Copy link
Member

@exercism/maintainers-admin, can we get an approval? Please and thank you.

@IsaacG
Copy link
Member

IsaacG commented Dec 4, 2024

+cc @ErikSchierboom

@iHiD
Copy link
Member

iHiD commented Dec 4, 2024

@exercism/maintainers-admin, can we get an approval? Please and thank you.

Currently - this is only @ErikSchierboom.

@ErikSchierboom Could you remove maintainers-admin from all the tooling repos please (now we have guardians). Then I can join the team and do this stuff alongside you? If I join the team now, I'll get subscribed to hundreds of repos which I don't want :)


In the meantime, I'll just merge this using admin priviledges.

@iHiD iHiD merged commit aac54c1 into exercism:main Dec 4, 2024
7 checks passed
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.

8 participants