Is narcissistic#6052
Is narcissistic#6052meg-1 wants to merge 32 commits intoTheAlgorithms:masterfrom meg-1:is_narcissistic
Conversation
ghost
added
require descriptive names
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
maths/is_narcissistic.py
Outdated
| @@ -0,0 +1,24 @@ | |||
| def is_narcissistic(i: int) -> bool: | |||
There was a problem hiding this comment.
Please provide descriptive name for the parameter: i
ghost
removed
the
require descriptive names
maths/average_welford.py
Outdated
| @@ -0,0 +1,21 @@ | |||
| def average_welford(values: list) -> float: | |||
There was a problem hiding this comment.
Please separate different algorithms into different PRs.
There was a problem hiding this comment.
thank you! i created new branches and comitted them seperately. it seemed to work. is it possible that i might've made a mistake while comitting?? i'm not sure how to check the result of the commit so i might've not seen that they commited as a group.
There was a problem hiding this comment.
So @meg-1, what you may try is to create 1 branch for 1 file (with its related changes if it applies). You may:
- checkout to the master branch
- switch to a new branch, say
average-welford git addandgit commitonlymaths/average_welford.py- check using
git status - push it to your repository; and open a PR for that branch
- repeat 1-5 for other files
If you have one commit for each file, it's very easy to do this with cherry-pick otherwise you'll need some rebase which is a bit complicated.
There was a problem hiding this comment.
Yes. They are (you can see it in Files changed tab). If you follow the push workflow above, you get one file in each branch, linked to one PR @meg-1
There was a problem hiding this comment.
Yes. They are (you can see it in
Files changedtab). If you follow the push workflow above, you get one file in each branch, linked to one PR @meg-1
ty for clarification. just checked and somehow they really did merge into one commit, 'is narcissistic' [ this pr, #6052 ]. i remember you said that the CI can sometimes not work properly, and i think this is the case for me. i did all prs exactly by the instruction steps which you recommended above, and it still merged into one pr. would you please suggest a further course of action, or any possible solutions, since i've been trying to commit for quite a long time, and even though i've been following all steps exactly, nothing seems to go through. @poyea
seperate pr for decrypt.py
trying to create seperate pr for decrypt.py
reasons: tests were failing, also wanted to divide commits.
[ there was also a syntax error here ]
added indentation to the code
|
@Kush1101 could you please review this pr as well? |
|
There's this version: https://github.com/TheAlgorithms/Python/blob/master/maths/armstrong_numbers.py. |
understood! apologies, didn't see that one. deleting #6052 right away. |
|
You may improve that current version, like adding more tests |
thank you, i will keep that in mind! |
2 participants
Describe your change:
Checklist:
Fixes: #{$ISSUE_NO}.