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: Stub generation testing and fixing of miscellaneous bugs #76

Merged
merged 84 commits into from
Mar 12, 2024

Conversation

Masara
Copy link
Contributor

@Masara Masara commented Mar 4, 2024

Summary of Changes

This PR contains the generated stub files from the Library project and is for testing those and fixing bugs, if they are found.

  • Collection[T] and Sequence[T] are recognized as TypeVars if used as superclasses
  • Mapping[] can be handled if used as superclass
  • Omit any class completely that inherits directly or transitively from Exception
  • Rework how reexports are handled for the is_public method (currently not working correctly)
  • Fix stub package names: "The shortest public "reexport" should be used based on the __init__.py files."

Masara and others added 19 commits February 28, 2024 16:25
# Conflicts:
#	tests/safeds_stubgen/stubs_generator/test_generate_stubs.py
# Conflicts:
#	src/safeds_stubgen/stubs_generator/_generate_stubs.py
#	tests/safeds_stubgen/stubs_generator/test_generate_stubs.py
…t be found if they were "hidden" behind an if-statement
…tubs, now we take the full path from the first __init__.py file occurrence, since this is the way python does it, too
Copy link

github-actions bot commented Mar 4, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 16 0 0 1.32s
✅ PYTHON mypy 16 0 5.77s
✅ PYTHON ruff 16 0 0 0.04s
✅ REPOSITORY git_diff yes no 0.02s

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

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 99.25373% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 99.21%. Comparing base (27659ee) to head (faadddd).

Files Patch % Lines
.../safeds_stubgen/stubs_generator/_generate_stubs.py 97.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
+ Coverage   98.82%   99.21%   +0.39%     
==========================================
  Files          25       25              
  Lines        2120     2176      +56     
==========================================
+ Hits         2095     2159      +64     
+ Misses         25       17       -8     

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

@Masara Masara marked this pull request as ready for review March 7, 2024 18:53
@Masara Masara requested review from a team and lars-reimann as code owners March 7, 2024 18:53
@Masara
Copy link
Contributor Author

Masara commented Mar 7, 2024

@lars-reimann Nevermind, your request regarding the package names still does not work according to your requirements. I thought that, for example, a module path/to/module which is exported in the path/to/__init__.py file like from path.to import module would get the stub text package path.to (only in this such a case). But according to you the package information is dependent on the class that is reexported in the __init__.py file, right? So if we have from path.to.module import SomeClass in the __init__.py instead of the first import I named, would we still get the stub text package path.to? And what if we have other public classes in the module that are not imported in the __init__ file?

@Masara
Copy link
Contributor Author

Masara commented Mar 7, 2024

But I'd say we close this PR first anyway, since the file changes are, once again, too much for a "quick" review.

@Masara
Copy link
Contributor Author

Masara commented Mar 8, 2024

So, after todays meeting there seemed to be a problem with these stubs. How would the stubs for the infer_alias_2 = AliasA part look like?

from data.modules import ClassA as AliasA

class ClassB:
   ...

alias_b = ClassB

class AliasingModuleClassC():
   typed_alias: alias_b
   infer_alias = alias_b()
   typed_alias_2: AliasA
   infer_alias_2 = AliasA
@PythonModule("data.modules.aliasing_module_1")
package data.modules.aliasingModule1

from data.modules import ClassA

class ClassB()

class AliasingModuleClassC() {
    @PythonName("typed_alias")
    static attr typedAlias: ClassB
    @PythonName("infer_alias")
    static attr inferAlias: ClassB
    @PythonName("typed_alias_2")
    static attr typedAlias2: ClassA
    @PythonName("infer_alias_2")
    static attr inferAlias2: ClassA
}

@lars-reimann
Copy link
Member

lars-reimann commented Mar 8, 2024

@lars-reimann Nevermind, your request regarding the package names still does not work according to your requirements. I thought that, for example, a module path/to/module which is exported in the path/to/__init__.py file like from path.to import module would get the stub text package path.to (only in this such a case). But according to you the package information is dependent on the class that is reexported in the __init__.py file, right? So if we have from path.to.module import SomeClass in the __init__.py instead of the first import I named, would we still get the stub text package path.to? And what if we have other public classes in the module that are not imported in the __init__ file?

Ask yourself how you can import a class/global function from a library into your Python code. There might be multiple ways to do so, if they get reexported in __init__.py files. Out of the possible ways, pick the public module path that is

  1. a subpath of the path to the module that declares the class/global function (if possible),
  2. the shortest possible one.

Then use that as the Safe-DS package name (after conversion to name convention).

Example:

# pack/_test.py

class MyClass
# pack/__init__.py

from ._test import MyClass
# Client code

from pack._test import MyClass # Legal, but not OK, since internal module
from pack import MyClass # OK

Resulting Safe-DS code:

package pack

class MyClass()

@lars-reimann
Copy link
Member

So, after todays meeting there seemed to be a problem with these stubs. How would the stubs for the infer_alias_2 = AliasA part look like?

from data.modules import ClassA as AliasA
class AliasingModuleClassC():
   infer_alias_2 = AliasA

We cannot represent the type of infer_alias_2 in the stubs, since we don't have a use case for reflection. Just omit the type and put a TODO above that line.

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.

Great work, thank you.

@lars-reimann lars-reimann merged commit 97b0ab3 into main Mar 12, 2024
8 checks passed
@lars-reimann lars-reimann deleted the stubs-testing-and-fixing branch March 12, 2024 19:27
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.

3 participants