Skip to content

Conversation

@tnguyen306
Copy link
Collaborator

@branhoff review please

@tnguyen306 tnguyen306 requested a review from branhoff February 9, 2023 04:49
@branhoff
Copy link
Owner

branhoff commented Feb 9, 2023

Quick note, can you add common virtual environment names to the .gitignore? I see the venv got committed. Same with the .DS_Store. That should be added to the .gitignore as well.

@branhoff
Copy link
Owner

branhoff commented Feb 9, 2023

The .DS_Store. still needs to be removed and then ignored for the future.

@branhoff
Copy link
Owner

branhoff commented Feb 9, 2023

I pointed out a few, but type hints are missing for many of the variables and nearly all of the parameters in the parameter list.

g = 9.8 # velocity

return 0.5 * g * t*t
def fall_distance(t) -> float:
Copy link
Owner

Choose a reason for hiding this comment

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

missing type hint on parameter t

Copy link
Owner

Choose a reason for hiding this comment

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

still missing type hint on parameter t

curr_num = 1
sol = 1
for i in range(n-1):
def fib(n) -> int:
Copy link
Owner

Choose a reason for hiding this comment

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

missing type hint on parameter n

Copy link
Owner

Choose a reason for hiding this comment

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

still missing type hint on parameter n

Represents a taxicab that tracks the distance traveled when given x and y coordinates
"""
def __init__(self, x, y):
def __init__(self, x, y) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

missing type hint on x and y parameters

Copy link
Owner

@branhoff branhoff left a comment

Choose a reason for hiding this comment

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

You may want to use mypy: https://www.mypy-lang.org/ to check that your type hinting is correct.

# Description: Mutates a given list and by the order of the elements of teh list

def reverse_list(lst):
def reverse_list(lst: any) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

I don't believe any is a type hint. The recommendation I ready is to call object for the Any class. This should be type hinted as list[object]

# Description: Mutates a given list and by the order of the elements of teh list

def reverse_list(lst):
def reverse_list(lst: object) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

this is type hinting a parameter named lst of any type. I think you mean lst: list[object]

"""
temp_lst = []
for i in range(len(lst)-1,-1, -1):
temp_lst: object = []
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. This should be temp_list: list[object] = []

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants