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

fix: Some packages couldn't be analyzed #51

Merged
merged 12 commits into from
Feb 21, 2024

Conversation

Masara
Copy link
Contributor

@Masara Masara commented Dec 10, 2023

Closes #48

Summary of Changes

  • Fixed the bug where some packages couldn't be analyzed because the Mypy build result could not be parsed from our side.
  • Added TypeVarType as a _types.py class for type variables.
  • Some bug fixes and refactoring

…ere some packages couldn't be analyzed b/c the mypy build result could not be parsed from our side; some bug fixes and refactoring
@Masara Masara linked an issue Dec 10, 2023 that may be closed by this pull request
@Masara Masara self-assigned this Dec 10, 2023
Copy link

codecov bot commented Dec 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a84ec64) 98.19% compared to head (070100d) 98.26%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   98.19%   98.26%   +0.07%     
==========================================
  Files          25       25              
  Lines        1882     1905      +23     
==========================================
+ Hits         1848     1872      +24     
+ Misses         34       33       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Dec 10, 2023

🦙 MegaLinter status: ⚠️ WARNING

Descriptor Linter Files Fixed Errors Elapsed time
✅ JSON eslint-plugin-jsonc 2 0 0 2.1s
✅ JSON jsonlint 2 0 0.28s
✅ JSON npm-package-json-lint yes no 0.58s
⚠️ JSON prettier 2 0 1 0.32s
✅ JSON v8r 2 0 6.28s
✅ PYTHON black 15 0 0 1.88s
✅ PYTHON mypy 15 0 8.1s
✅ PYTHON ruff 15 0 0 0.06s
✅ REPOSITORY git_diff yes no 0.01s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@Masara
Copy link
Contributor Author

Masara commented Dec 10, 2023

@lars-reimann I didn't know how to handle type variables like _T = TypeVar("_T") so for now I created a new type class in _types.py.

@Masara Masara marked this pull request as ready for review December 10, 2023 19:48
@Masara Masara requested a review from a team as a code owner December 10, 2023 19:48
@lars-reimann
Copy link
Member

lars-reimann commented Dec 11, 2023

What is the problem with those type variables? They were the only way to define classes with type parameters prior to Python 3.12 (PEP 695). So, there is no need to represent type variables directly, but only when they are used as type parameters. And there is no need to distinguish the different means to define type parameters.

@Masara
Copy link
Contributor Author

Masara commented Dec 15, 2023

@lars-reimann So, if a type parameter is defined like this:

_type_var = TypeVar("_type_var")

it should not be shown in the Safe-DS stubs if its not used?

And here the _type_var variable should be shown in the stubs:

_type_var = TypeVar("_type_var")
def type_var_func(type_var_list: list[_type_var]) -> list[_type_var]: ...

?

Also, how should the stubs for type variables look like? Like this?

@PythonName("_type_var")
typeVar = "_type_var"

@Pure
@PythonName("type_var_func")
fun typeVarFunc(typeVarList: List<typeVar>) -> result1: List<typeVar>

@lars-reimann
Copy link
Member

We treat type variables and the PEP 695 syntax identically. For the Python code you provided, the stubs would be

@Pure
@PythonName("type_var_func")
fun typeVarFunc<TypeVar>(typeVarList: List<TypeVar>) -> result1: List<TypeVar>

The syntax is identical to classes with type parameters: After the name you declare the type parameters and can then use them in the parameter and result list.

@Masara
Copy link
Contributor Author

Masara commented Feb 19, 2024

I moved the issue regarding the TypeVars to a new issue (#63) so we can close this PR. @lars-reimann could you check this PR and close?

@lars-reimann
Copy link
Member

I'll make room for this later today.

Copy link
Member

@lars-reimann lars-reimann left a comment

Choose a reason for hiding this comment

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

I'll approve this now, so we can move forward. In the future, however, please create PRs that fix a single bug/add a single feature and add an accurate description of what you try to accomplish. Otherwise, it's not possible to determine whether a PR really works.

@lars-reimann lars-reimann merged commit fa3d020 into main Feb 21, 2024
8 checks passed
@lars-reimann lars-reimann deleted the 48-currently-some-packages-cant-be-analyzed branch February 21, 2024 09:48
lars-reimann pushed a commit that referenced this pull request Mar 29, 2024
## [0.2.0](v0.1.0...v0.2.0) (2024-03-29)

### Features

* Added generation for Safe-DS stubs files ([#33](#33)) ([ab45b45](ab45b45))
* Correct stubs for TypeVars ([#67](#67)) ([df8c5c9](df8c5c9)), closes [#63](#63)
* Create stubs for public methods of inherited internal classes ([#69](#69)) ([71b38d7](71b38d7)), closes [#64](#64)
* Rework import generation for stubs. ([#50](#50)) ([216e179](216e179)), closes [#38](#38) [#24](#24) [#38](#38) [#24](#24)
* Safe-DS stubs also contain docstring information. ([#78](#78)) ([bdb43bd](bdb43bd))
* Stubs are created for referenced declarations in other packages ([#70](#70)) ([522f38d](522f38d)), closes [#66](#66)

### Bug Fixes

* Some packages couldn't be analyzed ([#51](#51)) ([fa3d020](fa3d020)), closes [#48](#48)
* Stub generation testing and fixing of miscellaneous bugs ([#76](#76)) ([97b0ab3](97b0ab3))
@lars-reimann
Copy link
Member

🎉 This PR is included in version 0.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lars-reimann lars-reimann added the released Included in a release label Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Included in a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Currently some packages can't be analyzed.
3 participants