Conversation
for more information, see https://pre-commit.ci
…ed comments for better understanding
for more information, see https://pre-commit.ci
algorithms-keeper
bot
added
awaiting reviews
for more information, see https://pre-commit.ci
|
add documentation |
|
How is this PR different from #8633? |
|
In order to merge a PR, all test must pass, can you try to fix these errors. |
CaedenPH
left a comment
CaedenPH
left a comment
There was a problem hiding this comment.
This change does not add any information that the person reading the code cannot infer themselves - Seems pointless
| if i not in temp: | ||
| temp.append(i) | ||
| ## changed below lines added sets functionality and remove for loops to get unique elemnt in list | ||
| temp = list(set(key)) |
There was a problem hiding this comment.
This is not a documentation change
| """This function takes two parameters: a string variable named | ||
| 'key' (with a default value of 'college') and a string variable | ||
| named 'pt' (with a default value of 'UNIVERSITY'). | ||
| The function returns a string that is a transformation of the 'pt' argument | ||
| based on a key-shift cipher""" |
There was a problem hiding this comment.
Firstly, this isn't inside of a function and isn't formatted
Secondly, what does this tell the user that we don't already know? Everything you said is clearly shown by typehints and naming conventions
| print(r) | ||
| print(len_temp) |
There was a problem hiding this comment.
Remove these prints
| print(r) | |
| print(len_temp) |
| # for i in range(r*len_temp): | ||
| # s = [temp[i] for _ in range(len_temp) if i < 26] | ||
| # modalpha.append(s) | ||
|
|
There was a problem hiding this comment.
| # for i in range(r*len_temp): | |
| # s = [temp[i] for _ in range(len_temp) if i < 26] | |
| # modalpha.append(s) |
Along with this random temp code
| """These lines of code creates a dictionary by iterating over the list of | ||
| lists. Each letter in the alphabets list is mapped to a letter in a row of | ||
| the "modalpha" list. The mappings are stored in the dictionary with the | ||
| indices of the alphabets list as keys and the values fromthe corresponding modalpha lists as values""" |
There was a problem hiding this comment.
These should be in the forms of comments, not docstrings
|
@CaedenPH this PR is basically identical to an earlier PR #8633 (same commits as this one), so one of the two should be closed. We can probably keep this PR since you already started a review on this one. There's also an even earlier PR #8626 by a different author that fixes the same issue #8622, so it'd be good if you could take a look at that one as well. |
tianyizheng02
left a comment
tianyizheng02
left a comment
There was a problem hiding this comment.
This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
Please make separate PRs for your changes to mixed_keyword_cypher.py and your changes to the other files
|
Closed as duplicate of #8633 |
4 participants
Describe your change:
Checklist:
Fixes: #{$ISSUE_NO}.