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

Improving static type checking results #11

Closed
Paebbels opened this issue Jan 26, 2022 · 3 comments
Closed

Improving static type checking results #11

Paebbels opened this issue Jan 26, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@Paebbels
Copy link
Member

@por3bski I wanted to ask you if you maybe have an idea how to improved the static type checking results for the code when it comes to using lxml.

I added lxml-stubs to the dependencies on dev branch, but still the results are not so good with respect to types from lxml.
See installation of lxml-stubs

Here is the mypy output: https://github.com/edaa-org/pyEDAA.UCIS/runs/4947176046?check_suite_focus=true#step:5:10
Here is the matching HTML report: https://edaa-org.github.io/pyEDAA.UCIS/typing/index.html

I'm also using lxml in other places, but never investigated what to improve in such a situation.

@por3bski
Copy link
Contributor

por3bski commented Jan 26, 2022

The main problem are types unions returned from lxml functions.
For example xpath(...) can return str, int, list, bool etc.

I have to use "assert" with isinstance() to narrow union types:

for scopeNode in scopes:
	assert isinstance(scopeNode, etree._Element) <----------

	if scopeNode.get("type").startswith("DU_"):
		continue

If this is accetable I can prepare PR in next few days.
I do not guarantee that it fixes every lxml error because lxml-stubs package is not complete:

These type annotations were initially included in typeshed, but lxml's annotations are still incomplete and have therefore been extracted from typeshed to avoid unintentional false positive results.

@Paebbels
Copy link
Member Author

I think I would prefer:

for scopeNode in scopes:
  if isinstance(scopeNode, etree._Element):  # <----------
    # some code here

  elif isinstance(scopeNode, etree.xxxxx):   # <----------
    # some code here

  else:
    raise ....                               # <----------

as a coding pattern instead of assert statements.


Because you have direct write access to this repository you can branch again from dev. You automatically get CI fully enabled. This generates all the reports like I linked them above. Then you can investigate differences and see how lxml behaves with some changes. If your commits improve the reports issues, you might go on and fix the next problems.

Note: You can also link your PR to this issues, so this issue gets closed when the PR is merged to the main branch :).

@umarcor
Copy link
Member

umarcor commented Jan 31, 2022

Implemented in #12 and merged to main in #10.

@umarcor umarcor closed this as completed Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

3 participants