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

Add DocTests to is_palindrome.py #10081

Merged

Conversation

SaiHarshaK
Copy link
Contributor

@SaiHarshaK SaiHarshaK commented Oct 8, 2023

Describe your change:

This PR adds DocTests to the functions in is_palindrome.py to increase its codecoverage from 0% to 96%.
Tested this change locally and can verify that they are passing.

Ref #9943

  • 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 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 enhancement This PR modified some existing files awaiting reviews This PR is ready to be reviewed labels Oct 8, 2023
@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Oct 8, 2023
@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Oct 8, 2023
data_structures/linked_list/is_palindrome.py Show resolved Hide resolved
data_structures/linked_list/is_palindrome.py Outdated Show resolved Hide resolved
Comment on lines +134 to +136
>>> is_palindrome_dict(\
ListNode(\
1, ListNode(2, ListNode(1, ListNode(3, ListNode(2, ListNode(1)))))))
Copy link
Member

Choose a reason for hiding this comment

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

PEP8 discourages backslash line termination in Python because any whitespace to the right of the backslash breaks the script on a change that is invisible to the reader. Also, backslashes are not required inside of (), [], {}...

Suggested change
>>> is_palindrome_dict(\
ListNode(\
1, ListNode(2, ListNode(1, ListNode(3, ListNode(2, ListNode(1)))))))
>>> is_palindrome_dict(
... ListNode(
... 1, ListNode(2, ListNode(1, ListNode(3, ListNode(2, ListNode(1)))))
... )
... )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ruff complains that the length of the line is >88 which is why I had to split them into multiple lines.

If i do not add backslash, DocTest assumes ListNode( is the expected value and thereby the test fails

Copy link
Member

Choose a reason for hiding this comment

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

You needed the three dots at the beginning of each continued line as discussed in the doctest docs.

@algorithms-keeper algorithms-keeper bot added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels Oct 8, 2023
@algorithms-keeper algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed tests are failing Do not merge until tests pass and removed awaiting changes A maintainer has requested changes to this PR labels Oct 8, 2023
@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Oct 8, 2023
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.

mypy takes some time to get used to but it is quite helpful in the end. If you do new_variable = None then it wants you to declare the datatype of new_variable because it refuses to guess.

data_structures/linked_list/is_palindrome.py Outdated Show resolved Hide resolved
data_structures/linked_list/is_palindrome.py Show resolved Hide resolved
@SaiHarshaK
Copy link
Contributor Author

@cclauss Please check now

@SaiHarshaK
Copy link
Contributor Author

Could you check the comment regarding backslashes?

@algorithms-keeper algorithms-keeper bot removed the awaiting reviews This PR is ready to be reviewed label Oct 9, 2023
@cclauss
Copy link
Member

cclauss commented Oct 9, 2023

I changed the title from doctests to type hints but this looks good to go... Thanks.

@cclauss cclauss merged commit 12e8e9c into TheAlgorithms:master Oct 9, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This PR modified some existing files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants