-
-
Notifications
You must be signed in to change notification settings - Fork 398
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
[#763] New practice exercise affine-cipher
#773
Conversation
config and generated tests.toml
Thank you for contributing to Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:
Automated comment created by PR Commenter 🤖. |
affine cipher
affine-cipher
config.json
Outdated
"tuples" | ||
], | ||
"practices": [ | ||
"enum", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I ran the checks on a slightly older branch that didn't have the concept tracking.
I'll remove enum
.
Maybe I could add pipe-operator
instead? AffineCipher.encode
is just one nice pipe.
Yes, a separate PR with just the bootstrap script would be great so that we don't need to block merging the exercise if the script need improvements or the other way around. I'll review only the exercise on this PR and wait for a new PR with the script 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work as always. You're unstoppable with the new exercises 😁.
I left a request to extend prerequisites and some optional feedback for the stub.
@doc """ | ||
Encode an encrypted message using a key | ||
""" | ||
@spec encode(key :: %{a: integer, b: integer}, message :: String.t()) :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there could be a custom type definition at the beginning of the module for the key?
"practices": [ | ||
"pipe-operator", | ||
"strings" | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot going on in this exercise, so I find it really hard to say what "practices" would be best. I think those make sense.
Co-authored-by: Angelika Tyborska <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍 I'll wait a bit before merging to see if @neenjaw wants to add a review too.
Probably don't have time to look at this today, I trust your judgement @angelikatyborska :D |
No worries! I understand that sometimes less review work is better, so I'll just merge and @neenjaw can submit a patch PR in case there's something he really wants changed 😀 |
One more exercise.
All files (minus
config.json
andtests.toml
) were generated automatically frombin/boostrap_practice_exercise.exs
. Of course, the tests needed a bunch of manual input, but usually at the level of search and replace, so it's quite valuable.I tried to be a little bit clever about some things (for example
@spec
generation for thelib
file) but I hope that contributors understand that they should not trust these guesses too much. Is it better to make them obviously wrong?Please test that script by running
elixir bin/boostrap_practice_exercise.exs darts
(or whichever exercise is not done yet, I have not tested for those yet) and inspect the generated files. I'm sure there is room for improvement.Now to think of it, should I make a different PR for this?