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.

Sign up for GitHub

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

Jump to bottom

Typing/models.readers #321

Merged
merged 9 commits into from
Apr 12, 2021
Merged

Typing/models.readers #321

merged 9 commits into from
Apr 12, 2021

Conversation

danieleades
Copy link
Contributor

@danieleades danieleades commented Mar 21, 2021 •
edited
Loading

Pull Request Check List

  • A news fragment is added in news/ describing what is new.
  • Test cases added for changed code.

Describe what you have changed in this PR.

refactor models.readers to improve typing support

@danieleades danieleades marked this pull request as draft March 21, 2021 14:15
@danieleades
Copy link
Contributor Author

models.readers.SetupReader has a really similar pattern to what's used in the iostream module - a class is used as a namespace for a bunch of stateless functions. Was tempted to turn these all into free functions, but i thought that might have been a bit drastic for this pull request.

@danieleades
Copy link
Contributor Author

danieleades commented Mar 21, 2021 •
edited
Loading

here are the remaining type errors in this file (mypy pdm/ | grep readers)

pdm/models/readers.py:84: error: Incompatible types in assignment (expression has type "Optional[List[Any]]", variable has type "List[stmt]")
pdm/models/readers.py:171: error: Unsupported left operand type for + ("None")
pdm/models/readers.py:171: note: Left operand is of type "Optional[List[Any]]"
pdm/models/readers.py:185: error: "expr" has no attribute "id"
pdm/models/readers.py:205: error: Incompatible types in assignment (expression has type "Optional[Call]", variable has type "Tuple[Optional[Call], Optional[List[Any]]]")
pdm/models/readers.py:207: error: Unsupported operand types for + ("List[Any]" and "None")
pdm/models/readers.py:207: note: Right operand is of type "Optional[List[Any]]"
pdm/models/readers.py:209: error: Incompatible return value type (got "Tuple[Tuple[Optional[Call], Optional[List[Any]]], List[Any]]", expected "Tuple[Optional[Call], Optional[List[Any]]]")
pdm/models/readers.py:215: error: Need type annotation for 'install_requires' (hint: "install_requires: List[<type>] = ...")
pdm/models/readers.py:244: error: "expr" has no attribute "s"
pdm/models/readers.py:250: error: "expr" has no attribute "s"
pdm/models/readers.py:258: error: Need type annotation for 'extras_require' (hint: "extras_require: Dict[<type>, <type>] = ...")
pdm/models/readers.py:288: error: Incompatible types in assignment (expression has type "Optional[Any]", variable has type "expr")
pdm/models/readers.py:291: error: Item "expr" of "Optional[expr]" has no attribute "s"
pdm/models/readers.py:291: error: Item "None" of "Optional[expr]" has no attribute "s"
pdm/models/readers.py:291: error: "expr" has no attribute "s"
pdm/models/readers.py:300: error: Incompatible types in assignment (expression has type "Optional[Any]", variable has type "expr")
pdm/models/readers.py:303: error: Item "expr" of "Optional[expr]" has no attribute "s"
pdm/models/readers.py:303: error: Item "None" of "Optional[expr]" has no attribute "s"
pdm/models/readers.py:303: error: "expr" has no attribute "s"

might need some help delving into these. they look they mostly from missing 'None' checks

frostming
frostming reviewed Mar 21, 2021
View reviewed changes
Copy link
Collaborator

@frostming frostming left a comment

Choose a reason for hiding this comment

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

I think staticmethod or classmethod is good for this case, depending on the method sematic.

pdm/models/readers.py

@classmethod
def from_directory(cls, dir: Path) -> "Setup":
return _SetupReader.read_from_directory(dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why prefer exposing Setup instead of SetupReader? You know, it may involve changing the reference in other modules as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because i'm now exposing a strongly-typed 'Setup' class instead of a stringly-typed dictionary.

I've updated both external modules that reference this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these changes bring the number of mypy errors down from 44 to 20. Still a few to go though! - #321 (comment)

in the previous implementation the 'setup' is stored in a Dict[str, Any], which means type inference doesn't work in any function that indexes any of the fields.

Now I could still have imported the SetupReader in other modules instead of Setup, but i think this is a neater abstraction. the from_directory static method on Setup is effectively a constructor, and the calling code doesn't need to know anything about the SetupReader, which is now effectively just a helper class

@danieleades danieleades force-pushed the typing/models.readers branch 2 times, most recently from 4dc823b to 2118a32 Compare March 21, 2021 14:57
@danieleades danieleades mentioned this pull request Mar 21, 2021
Closed
6 tasks
@danieleades danieleades force-pushed the typing/models.readers branch from 3f68cdd to 7f86f0d Compare March 22, 2021 21:30
@danieleades
Copy link
Contributor Author

rebased on master

@danieleades
Copy link
Contributor Author

all the remaining type errors in this file are related to the setup.py parsing. I'm not exactly sure how to proceed, given the black magic here...

pdm/models/setup.py:88: error: Incompatible types in assignment (expression has type "Optional[List[Any]]", variable has type "List[stmt]")
pdm/models/setup.py:175: error: Unsupported left operand type for + ("None")
pdm/models/setup.py:175: note: Left operand is of type "Optional[List[Any]]"
pdm/models/setup.py:189: error: "expr" has no attribute "id"
pdm/models/setup.py:209: error: Incompatible types in assignment (expression has type "Optional[Call]", variable has type "Tuple[Optional[Call], Optional[List[Any]]]")
pdm/models/setup.py:211: error: Unsupported operand types for + ("List[Any]" and "None")
pdm/models/setup.py:211: note: Right operand is of type "Optional[List[Any]]"
pdm/models/setup.py:213: error: Incompatible return value type (got "Tuple[Tuple[Optional[Call], Optional[List[Any]]], List[Any]]", expected "Tuple[Optional[Call], Optional[List[Any]]]")
pdm/models/setup.py:248: error: "expr" has no attribute "s"
pdm/models/setup.py:254: error: "expr" has no attribute "s"
pdm/models/setup.py:292: error: Incompatible types in assignment (expression has type "Optional[Any]", variable has type "expr")
pdm/models/setup.py:295: error: Item "expr" of "Optional[expr]" has no attribute "s"
pdm/models/setup.py:295: error: Item "None" of "Optional[expr]" has no attribute "s"
pdm/models/setup.py:295: error: "expr" has no attribute "s"
pdm/models/setup.py:304: error: Incompatible types in assignment (expression has type "Optional[Any]", variable has type "expr")
pdm/models/setup.py:307: error: Item "expr" of "Optional[expr]" has no attribute "s"
pdm/models/setup.py:307: error: Item "None" of "Optional[expr]" has no attribute "s"
pdm/models/setup.py:307: error: "expr" has no attribute "s"

@danieleades danieleades force-pushed the typing/models.readers branch 2 times, most recently from b0f9c91 to 2784aed Compare March 24, 2021 16:44
@frostming frostming changed the base branch from master to refactor/typing March 31, 2021 07:30
@frostming frostming changed the base branch from refactor/typing to master March 31, 2021 07:30
@danieleades danieleades force-pushed the typing/models.readers branch from 2784aed to 875163c Compare April 11, 2021 12:47
@danieleades
Copy link
Contributor Author

rebased on master

@danieleades
Copy link
Contributor Author

i've added decorators to ignore the typing errors in some of these functions. it's not an ideal solution, but it will suppress the errors and get you a step closer to using mypy in CI/pre-commit.

seems like parsing setup.py files should be a reasonably common problem. are there no third-party libraries that can be used?

@danieleades danieleades marked this pull request as ready for review April 11, 2021 13:05
@frostming frostming merged commit 729d66c into pdm-project:master Apr 12, 2021
@danieleades danieleades deleted the typing/models.readers branch April 12, 2021 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frostming frostming frostming approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danieleades @frostming