Skip to content

/ Python

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

Implemented static type checking to avl_tree.py #2295

Open
wants to merge 4 commits into
base: master
from
Open

Implemented static type checking to avl_tree.py #2295

wants to merge 4 commits into from

Conversation

@anushkrishnav
Copy link

anushkrishnav commented Aug 10, 2020 •
edited

Describe your change:

  • 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.
  • 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}.
    I am more than willing to implement static type checking for other program files too . I was inspired by PR #2293
@anushkrishnav
Implemented static type checking to avl_tree.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
0387f5e
@TravisBuddy
Copy link

TravisBuddy commented Aug 10, 2020

Hey @anushkrishnav,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: c07007a0-dad8-11ea-bee2-571e9d389dd6
@anushkrishnav
Fixed Linting issue
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
86e2b12
@cclauss
cclauss reviewed Aug 10, 2020
View changes
data_structures/binary_tree/avl_tree.py Outdated
@@ -11,15 +11,15 @@


class my_queue:

This comment has been minimized.

Sign in to view
@cclauss

cclauss Aug 10, 2020

Member
Suggested change
class my_queue:
class MyQueue:

Proper Python class naming. See CONTRIBUTING.md.

This comment has been minimized.

Sign in to view
@anushkrishnav

anushkrishnav Aug 10, 2020

Author

I just implemented the static type checking on an exsisting document in the repo . But I would do the suggested changes .
@cclauss
cclauss reviewed Aug 10, 2020
View changes
data_structures/binary_tree/avl_tree.py Outdated
@@ -31,18 +31,22 @@ def pop(self):
def count(self):
return self.tail - self.head

def print(self):
def print(self) -> None:

This comment has been minimized.

Sign in to view
@cclauss

cclauss Aug 10, 2020

Member

This is a bad method name because it shadows a Python builtin function. Why not rename this to str() and then just print(my_queue_instance)?

This comment has been minimized.

Sign in to view
@anushkrishnav

anushkrishnav Aug 10, 2020 •
edited

Author

Do you mean a string representation of the object? Thats sounds more reasonable .Wll correct it

This comment has been minimized.

Sign in to view
@cclauss

cclauss Aug 10, 2020 •
edited

Member

https://docs.python.org/3/reference/datamodel.html#object.__str__
@cclauss
cclauss reviewed Aug 10, 2020
View changes
data_structures/binary_tree/avl_tree.py Outdated
def set_data(self, data):
self.data = data
def set_data(self, data: int) -> None:
self.data: int = data

This comment has been minimized.

Sign in to view
@cclauss

cclauss Aug 10, 2020

Member

Not required because of the previous line.
@cclauss
cclauss reviewed Aug 10, 2020
View changes
data_structures/binary_tree/avl_tree.py Outdated
@@ -31,18 +31,22 @@ def pop(self):
def count(self):
return self.tail - self.head

def print(self):
def print(self) -> None:
print(self.data)
print("**************")
print(self.data[self.head : self.tail])


class my_node:

This comment has been minimized.

Sign in to view
@cclauss

cclauss Aug 10, 2020

Member
Suggested change
class my_node:
class Node:
@cclauss
cclauss reviewed Aug 10, 2020
View changes
data_structures/binary_tree/avl_tree.py
print("insert:" + str(data))
self.root = insert_node(self.root, data)

def del_node(self, data):
def del_node(self, data: int) -> None:
print("delete:" + str(data))
if self.root is None:
print("Tree is empty!")

This comment has been minimized.

Sign in to view
@cclauss

cclauss Aug 10, 2020

Member

Algorithmic functions/methods should not print() as discussed in CONTRIBUTING.md. Please raise an appropriate exception instead.
@cclauss
cclauss requested changes Aug 10, 2020
View changes
Copy link
Member

cclauss left a comment •
edited

AVLTree has doctests but the other classes need them as well as discussed in CONTRIBUTING.md.
@anushkrishnav
Fixed class name and removed unwanted check
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
fe39395
@TravisBuddy
Copy link

TravisBuddy commented Aug 10, 2020

Hey @anushkrishnav,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 07df93f0-daff-11ea-bc40-19e33457b5db
@anushkrishnav
Remove an extra blank line
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
7881402
@anushkrishnav
Copy link
Author

anushkrishnav commented Aug 28, 2020

AVLTree has doctests but the other classes need them as well as discussed in CONTRIBUTING.md.

Okay Working on it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cclauss cclauss

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@anushkrishnav @TravisBuddy @cclauss
You can’t perform that action at this time.