Skip to content

[mypy] fix directory "dynamic_programming"#4323

Closed
algobytewise wants to merge 3 commits intoTheAlgorithms:masterfrom
algobytewise:mypy-fix-dynamic_programming
Closed

[mypy] fix directory "dynamic_programming"#4323
algobytewise wants to merge 3 commits intoTheAlgorithms:masterfrom
algobytewise:mypy-fix-dynamic_programming

Conversation

@algobytewise
Copy link
Copy Markdown
Contributor

Describe your change:

Related Issue: #4052

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

@algobytewise algobytewise requested a review from Kush1101 as a code owner April 7, 2021 10:05
@ghost ghost added enhancement This PR modified some existing files awaiting reviews This PR is ready to be reviewed labels Apr 7, 2021
@ghost ghost added tests are failing Do not merge until tests pass and removed tests are failing Do not merge until tests pass labels Apr 7, 2021
@dhruvmanila
Copy link
Copy Markdown
Member

We are not only correcting the mypy errors, but actually providing type hints where they are missing like in: https://github.com/TheAlgorithms/Python/blob/master/dynamic_programming/bitmask.py#L15

I was already working on dynamic_programming directory and have fixed around 13 files. Our goal is not to satisfy mypy but actually get the entire codebase statically typed.

value = [int(v) for v in value]
weight = input(f"Enter the positive weights of the {n} item(s) in order: ".split())
weight = [int(w) for w in weight]
valueStrings = input(f"Enter the values of the {n} item(s) in order: ").split()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Python follows the snake_case naming convention.

Comment thread mypy.ini Outdated

; FIXME: #4052 fix mypy errors in the exclude directories and remove them below
exclude = (data_structures|dynamic_programming|graphs|maths|matrix|other|project_euler|searches|strings*)/$
exclude = (data_structures|graphs|maths|matrix|other|project_euler|searches|strings*)/$
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove this change as the directory is not completely typed.

Copy link
Copy Markdown
Contributor Author

@algobytewise algobytewise Apr 8, 2021

Choose a reason for hiding this comment

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

If we want to make sure that there are no untyped functions, we could use the additional parameter --disallow-untyped-defs, see https://mypy.readthedocs.io/en/stable/command_line.html#untyped-definitions-and-calls.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you looking into it. We are going to make mypy stricter, but not now.

@algobytewise
Copy link
Copy Markdown
Contributor Author

algobytewise commented Apr 8, 2021

Just to clarify: with getting "the entire codebase statically typed" I assume you mean all the inputs & outputs of functions and not truly everything.

@dhruvmanila
Copy link
Copy Markdown
Member

dhruvmanila commented Apr 8, 2021

I do mean everything which mypy cannot infer itself. Some variables will be difficult for mypy to infer the type of for which we should provide the type. Common examples include the container types like list, dict, etc.

Also, we should be using duck typings, generics and type variables where possible. Duck typings include the ones which define a certain behavior type and not a concrete type like Iterable, Container, Sequence, etc. Generics and type variables are Generic and TypeVar from the typing module.

@algobytewise
Copy link
Copy Markdown
Contributor Author

Ah I see, that's good to know.

@algobytewise
Copy link
Copy Markdown
Contributor Author

Could someone have a look at this contribution? It fixes all the errors (mypy --ignore-missing-imports .) but it doesn't fix all the missing type annotations (mypy --ignore-missing-imports --disallow-untyped-defs .). In the meantime there has been another pull request for fixing this directory: #4332. Not sure which one is a better candidate for the merge.

@stale
Copy link
Copy Markdown

stale Bot commented Jun 16, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the stale Used to mark an issue or pull request stale. label Jun 16, 2021
@stale
Copy link
Copy Markdown

stale Bot commented Jun 26, 2021

Please reopen this pull request once you commit the changes requested or make improvements on the code. If this is not the case and you need some help, feel free to seek help from our Gitter or ping one of the reviewers. Thank you for your contributions!

@stale stale Bot closed this Jun 26, 2021
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 enhancement This PR modified some existing files stale Used to mark an issue or pull request stale.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants