Skip to content

Readable code, function docstring and doctests#10888

Closed
Hardvan wants to merge 14 commits intoTheAlgorithms:masterfrom
Hardvan:fractional-knapsack
Closed

Readable code, function docstring and doctests#10888
Hardvan wants to merge 14 commits intoTheAlgorithms:masterfrom
Hardvan:fractional-knapsack

Conversation

@Hardvan
Copy link
Contributor

@Hardvan Hardvan commented Oct 24, 2023 •
edited
Loading

Improved code readability by using separate if-elif statements for conditional checks.
Added inline comments to describe the procedure steps.
Enhanced function documentation for clarity.
Incorporated additional function doctests for thorough testing.
Added Wikipedia reference link for the Fractional Knapsack algorithm.

  • 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".

@cclauss cclauss self-assigned this Oct 24, 2023
cclauss
cclauss requested changes Oct 24, 2023
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.

This is checked on the commit message but these changes do NOT comply.

Please avoid changing both code and tests in a single pull request.

@algorithms-keeper algorithms-keeper bot added the awaiting changes A maintainer has requested changes to this PR label Oct 24, 2023
@algorithms-keeper algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed and removed awaiting changes A maintainer has requested changes to this PR labels Oct 24, 2023
@Hardvan Hardvan requested a review from cclauss October 24, 2023 11:38
@Hardvan
Copy link
Contributor Author

Hardvan commented Oct 24, 2023

I have removed the added doctests. Now, there is only one change - the code.

@cclauss
Copy link
Member

cclauss commented Oct 24, 2023

Great. Please open another pull request with the doctests and we can merge that one before this one.

@Hardvan Hardvan mentioned this pull request Oct 24, 2023
Merged
15 tasks
@Hardvan
Copy link
Contributor Author

Hardvan commented Oct 24, 2023

I have opened the PR #10891 which has the added doctests. Thanks!

@algorithms-keeper algorithms-keeper bot mentioned this pull request Oct 24, 2023
Closed
15 tasks
@cclauss
Copy link
Member

cclauss commented Oct 24, 2023

OK. Please add the tests that have been merged to master.

@Hardvan
Copy link
Contributor Author

Hardvan commented Oct 24, 2023

Added the tests that have been merged to master.

cclauss
cclauss reviewed Oct 24, 2023
View reviewed changes
cclauss
cclauss reviewed Oct 24, 2023
View reviewed changes
Hardvan and others added 3 commits October 24, 2023 21:57
@Hardvan @cclauss
Add strict=True
4d24580 
Co-authored-by: Christian Clauss <cclauss@me.com>
@Hardvan @cclauss
Add self-documenting variable names
b0a5d6d 
Co-authored-by: Christian Clauss <cclauss@me.com>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
2fb82e2 
for more information, see https://pre-commit.ci
@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Oct 24, 2023
@Hardvan
Copy link
Contributor Author

Hardvan commented Oct 24, 2023 •
edited
Loading

Renamed the variables.
Adding strict=True produces this error:
TypeError: sort() takes at most 2 keyword arguments (3 given)
Edit: Fixed the above error by adding strict=True in the zip function

@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Oct 24, 2023
@Hardvan Hardvan force-pushed the fractional-knapsack branch from 05d92c0 to aa93e55 Compare October 24, 2023 16:35
@Hardvan Hardvan requested a review from cclauss October 24, 2023 16:39
cclauss
cclauss reviewed Oct 24, 2023
View reviewed changes
@Hardvan
Copy link
Contributor Author

Hardvan commented Oct 24, 2023

I'll add the changes tomorrow. Thanks for highlighting the issues.

@Hardvan
Copy link
Contributor Author

Hardvan commented Oct 25, 2023

I have added the input validation check for capacity and max_items variables, and updated the doctests accordingly.

cclauss
cclauss reviewed Oct 25, 2023
View reviewed changes
@Hardvan
Copy link
Contributor Author

Hardvan commented Oct 25, 2023

I have corrected the algorithm.

@Hardvan Hardvan requested a review from cclauss October 25, 2023 14:43
@algorithms-keeper algorithms-keeper bot mentioned this pull request Sep 30, 2024
Closed
15 tasks
@Hardvan Hardvan closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cclauss cclauss Awaiting requested review from cclauss

Assignees

@cclauss cclauss

Labels

awaiting reviews This PR is ready to be reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Hardvan @cclauss