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

Fixes wiki and doctest issues #11227

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

MarlieChiller
Copy link

@MarlieChiller MarlieChiller commented Jan 2, 2024

Describe your change:

Fixes a markdown typo in the wiki (unable to edit the wiki directly in a PR so I think @Siddharth1605 added the files as new to be ported). In doing so, the ci doctests pipeline failed for calculate_age in web_programming/get_top_billionaires.py which I refactored and addressed

[closes #11225]

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Add or change doctests? -- Note: Please avoid changing both code and tests in a single pull request.
  • 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 include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the description above includes the issue number(s) with a closing keyword: "Fixes #ISSUE-NUMBER".

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Jan 2, 2024
@algorithms-keeper algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed and removed tests are failing Do not merge until tests pass labels Jan 2, 2024
@MarlieChiller
Copy link
Author

MarlieChiller commented Jan 2, 2024

I havent taken the time to fully understand the code in the calculate_age function but the doctests seem a bit brittle here. It doesnt seem coincidence that these fail just after a new year starts. A little strange that its a smaller number rather than a larger number as well but that could be down to my limited undestanding

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Jan 2, 2024
@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Jan 2, 2024
@MarlieChiller MarlieChiller changed the title Fixes wiki and doctest issues in python repo Fixes wiki and doctest issues Jan 2, 2024
@@ -0,0 +1,6 @@
## Python Algorithms
Copy link
Author

Choose a reason for hiding this comment

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

Yes, I suppose that is true. In fixing the wiki issue, the doctests issue was highlighted. Both are addressed here

<br/>

<ol>
<li><a href="">Selection Sort</a></li>
Copy link
Member

@cclauss cclauss Jan 2, 2024

Choose a reason for hiding this comment

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

I am a bit uncomfortable with an unsorted list of sorting algorithms. Are these sorted alphabetically, sorted for speed, or other? DIRECTORY.md is machine-generated to keep algorithms up-to-date and sorted alphabetically.

Copy link
Author

@MarlieChiller MarlieChiller Jan 2, 2024

Choose a reason for hiding this comment

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

This is a like for like drop-in except for the corrected markdown. The existing pages algorithms were already sorted like this. Ultimately, I was just trying to fix the issue highlighted in #11227 which also has the unsorted list. The wiki can be edited in the GUI by a repo admin in <30seconds

<li><a href="">Binary Insertion Sort</a></li>
<li><a href="">BogoSort or Permutation Sort</a></li>
<li><a href="">Gnome Sort</a></li>
<li><a href="">Sleep Sort – The King of Laziness / Sorting while Sleeping</a></li>
Copy link
Member

Choose a reason for hiding this comment

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

I HATE this algorithm because it wastes time and compute power.

web_programming/get_top_billionaires.py Outdated Show resolved Hide resolved
MarlieChiller and others added 3 commits January 3, 2024 11:50
Fixed calculate_age function such that is returns the age correctly from a given timestamp. Fixed doctests to be robust when year changes
removed unused import
@MarlieChiller
Copy link
Author

MarlieChiller commented Jan 3, 2024

@cclauss Ive updated the whole calculate_age function and related doctests as I dont think it was achieving what it set out to do initially. Please review changes when appropriate. I can see new PRs against this repo are now failing due to this function and its doctests in master branch

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Jan 15, 2024
@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Jan 15, 2024
@cclauss
Copy link
Member

cclauss commented Jan 15, 2024

The get_top_billionaires() issue was fixed in #11234 so please revert all changes to that file.

@cclauss
Copy link
Member

cclauss commented Jan 15, 2024

I am lost trying to figure out how this works. If I go to https://github.com/TheAlgorithms/Python/wiki I already get most of this. Could you let me know why what we have already is not enough?

When I click on List of Algorithms and scroll down to Sorts I get https://github.com/TheAlgorithms/Python/blob/master/DIRECTORY.md#sorts which is an alphabetized list of all sort algorithms that is kept up to date by https://github.com/TheAlgorithms/Python/blob/master/.github/workflows/directory_writer.yml every time a pull request is created. Why do we want a manual, unsorted list of sorts? How is that a better user experience?

It is more customary and easier to find if the information about best case, worst case, is documented in
https://github.com/TheAlgorithms/Python/blob/master/sorts/README.md

@MarlieChiller
Copy link
Author

This PR is literally only to fix the typos that I found in the markdown of the wiki as mentioned in 11225. You only need to correct those typos now as you have fixed the other changes that I spent time correcting in get_top_billionaires.py.

@MarlieChiller
Copy link
Author

MarlieChiller commented Jan 15, 2024

To put it most simply, please visit https://github.com/TheAlgorithms/Python/wiki/Sorting-Algorithms#selection-sort. It is misformatted and links to the wrong algorithm (bubble sort rather than selection sort). You can lift the relevant markdown from this PR directly into the wiki to resolve this issue. Github has no way of easily opening PRs against wiki pages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting reviews This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misformatted/linked wiki entry in python sorting
3 participants