Skip to content

Added Dutch National Flag algorithm #4636 #4639

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Aug 31, 2021
Merged

Added Dutch National Flag algorithm #4636 #4639

merged 12 commits into from
Aug 31, 2021

Conversation

KiranHipparagi
Copy link
Contributor

Describe your change:

Added Dutch National Flag algorithm

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@ghost ghost added awaiting reviews This PR is ready to be reviewed require descriptive names This PR needs descriptive function and/or variable names require tests Tests [doctest/unittest/pytest] are required require type hints https://docs.python.org/3/library/typing.html labels Aug 19, 2021
ghost
ghost reviewed Aug 19, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

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 review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to 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.

@KiranHipparagi
Added descriptive name and type hint
3cd9ba1 
Added descriptive name and type hint  for parameter with doctest for the function dnf_sort.
@ghost ghost removed require descriptive names This PR needs descriptive function and/or variable names require type hints https://docs.python.org/3/library/typing.html labels Aug 20, 2021
ghost
ghost reviewed Aug 20, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

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 review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to 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.

ghost
ghost reviewed Aug 20, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

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 review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to 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.

ghost
ghost reviewed Aug 20, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

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 review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to 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.

@KiranHipparagi
Copy link
Contributor Author

@cclauss @dhruvmanila please review

@ghost ghost added the tests are failing Do not merge until tests pass label Aug 31, 2021
@cclauss
Copy link
Member

cclauss commented Aug 31, 2021 •
edited
Loading

Please format your code with psf/black as discussed in CONTRIBUTING.md.

dnf_sort() needs doctests including input with a -1, 3, 1.1, and "A" to see how the function deals with bad input.

@KiranHipparagi
Copy link
Contributor Author

@cclauss The elements inside the sequence must contains only [0, 1, 2] values as per dutch national flag standard.But in future we can add other features also..

@cclauss
Copy link
Member

cclauss commented Aug 31, 2021 •
edited
Loading

The elements inside the sequence must contains only [0, 1, 2]

Yes, but the tests need to demonstrate how the algorithm deals with bad data as discussed in CONTRIBUTING.md.

cclauss
cclauss reviewed Aug 31, 2021
View reviewed changes
@KiranHipparagi
Copy link
Contributor Author

@cclauss soory this is my first contribution.i have gone through CONTRIBUTING.md and followed all the steps.Not getting what exactly needs to change. could you please give me link if you have anything so that i'll check and correct it. Thanks

@cclauss
Copy link
Member

cclauss commented Aug 31, 2021

$ mypy .

sorts/dnf_sort.py:47: error: Return value expected
Found 1 error in 1 file (checked 318 source files)

@cclauss
Copy link
Member

cclauss commented Aug 31, 2021

Click the details link next to the ❌es below to see what tests are failing.

cclauss
cclauss requested changes Aug 31, 2021
View reviewed changes
@ghost ghost added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels Aug 31, 2021
ghost
ghost reviewed Aug 31, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

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 review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to 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.

@ghost ghost removed the awaiting changes A maintainer has requested changes to this PR label Aug 31, 2021
@ghost ghost added the awaiting reviews This PR is ready to be reviewed label Aug 31, 2021
ghost
ghost reviewed Aug 31, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

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 review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to 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.

cclauss
cclauss reviewed Aug 31, 2021
View reviewed changes
ghost
ghost reviewed Aug 31, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

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 review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to 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.

@KiranHipparagi
Copy link
Contributor Author

@cclauss thank you i'll make correction.

@KiranHipparagi
Added doctest for the function
c35debb 
dutch_national_flag_sort
@ghost ghost removed the require tests Tests [doctest/unittest/pytest] are required label Aug 31, 2021
@KiranHipparagi
Copy link
Contributor Author

@cclauss updated doctest.Thanks for the clarification.

@KiranHipparagi KiranHipparagi requested a review from cclauss August 31, 2021 12:52
@cclauss
Copy link
Member

cclauss commented Aug 31, 2021 •
edited
Loading

https://github.com/TheAlgorithms/Python/pull/4639/checks?check_run_id=3473281739 is a ❌ below. The - lines are your code and the + lines are how https://pypi.org/project/black would reformat your code.

All lines in your code should be 88 characters max or you will continue to see ❌es.

@KiranHipparagi
Copy link
Contributor Author

https://github.com/TheAlgorithms/Python/pull/4639/checks?check_run_id=3473281739 is a x below. The - lines are your code and the + lines are how https://pypi.org/project/black would reformat your code.

All lines in your code should be 88 characters max or you will continue to see xes.

updated file as per black code formatter.

@KiranHipparagi
Copy link
Contributor Author

KiranHipparagi commented Aug 31, 2021 •
edited
Loading

@cclauss please review updated code.

@ghost ghost removed the tests are failing Do not merge until tests pass label Aug 31, 2021
cclauss
cclauss approved these changes Aug 31, 2021
View reviewed changes
Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks for your persistence. It is a cool algorithm.

@ghost ghost removed the awaiting reviews This PR is ready to be reviewed label Aug 31, 2021
@cclauss cclauss merged commit 757d4fb into TheAlgorithms:master Aug 31, 2021
shermanhui pushed a commit to shermanhui/Python that referenced this pull request Oct 22, 2021
@KiranHipparagi @cclauss @shermanhui
Added Dutch National Flag algorithm TheAlgorithms#4636 (TheAlgorithms…
bea4c39 
…#4639)

* Added Dutch national flag sort Algorithm

* Changed file name to dnf_sort.py

* Added descriptive name and type hint 

Added descriptive name and type hint  for parameter with doctest for the function dnf_sort.

* Added test cases

* Added doctest cases

* Update sorts/dnf_sort.py

* Added doctest for dutch_national_flag_sort sorts/dnf_sort.py

* Update sorts/dnf_sort.py

* Added doctest for the function
dutch_national_flag_sort

* update file as per black code formatter

* Update dnf_sort.py

* Update and rename dnf_sort.py to dutch_national_flag_sort.py

Co-authored-by: Christian Clauss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cclauss cclauss cclauss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KiranHipparagi @cclauss