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: Fixed a bug where imports would not check reexports for shortest path #112

Merged
merged 27 commits into from
May 4, 2024

Conversation

Masara
Copy link
Contributor

@Masara Masara commented Apr 13, 2024

Closes #82

Summary of Changes

Fixed the bug where imports would not use the shortest path from existing reexports.

@Masara Masara requested a review from a team as a code owner April 13, 2024 19:29
@Masara Masara linked an issue Apr 13, 2024 that may be closed by this pull request
Copy link

codecov bot commented Apr 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.55%. Comparing base (cb061ab) to head (9ea4421).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   99.53%   99.55%   +0.01%     
==========================================
  Files          23       23              
  Lines        2373     2462      +89     
==========================================
+ Hits         2362     2451      +89     
  Misses         11       11              

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

Copy link

github-actions bot commented Apr 13, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 5 0 0 1.23s
✅ PYTHON mypy 5 0 5.57s
✅ PYTHON ruff 5 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

@Masara Masara force-pushed the 82-wrong-imports-in-created-stubs branch from e2356d7 to 7641bc3 Compare April 22, 2024 00:15
… & Alias names are now correctly taken over to stubs and whitelist reexports are now handled correctly
@Masara Masara force-pushed the 82-wrong-imports-in-created-stubs branch from a2bd81a to 49c668a Compare April 22, 2024 12:24
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.

On this branch, no stubs are created at all for Safe-DS:

image

@Masara
Copy link
Contributor Author

Masara commented Apr 25, 2024

For me stubs are created. Did you remove the -p argument from the call? The last PR merge removed the --package argument for the api call

@lars-reimann
Copy link
Member

Yep, same call as for main, where it works fine: safe-ds-stubgen -o "out" -nc -s "../library/src" --docstyle NUMPYDOC -v

Did you delete your out folder to ensure that new files are actually being created?

…e the imports names where converted too soon
@Masara
Copy link
Contributor Author

Masara commented Apr 26, 2024

I did find and fix a bug here, which wouldn't let the parser create stubs for the latest main (same bug as mentioned here.)
Could you try again?

@lars-reimann
Copy link
Member

@Masara Please take a look at the merge conflicts. Then this should also be good to go.

# Conflicts:
#	src/safeds_stubgen/stubs_generator/_generate_stubs.py
#	tests/safeds_stubgen/stubs_generator/test_generate_stubs.py
@lars-reimann
Copy link
Member

lars-reimann commented May 2, 2024

Unfortunately, this is broken for me again. I'm running safe-ds-stubgen -o "./out" -nc -s "E:/Repositories/safe-ds/Library/src" --docstyle NUMPYDOC -v, as before, but now my output looks like this:

image

  • safeds/data/image/containers/containers.sdsstub contains the full classes Image and ImageList.
  • safeds/data/tabular/containers/containers.sdsstub only contains this:
package safeds.data.tabular.containers

class Table

Imports aren't being created at all, so I cannot check whether that works now.

@Masara Masara force-pushed the 82-wrong-imports-in-created-stubs branch from 8d7ef3b to 47730e2 Compare May 3, 2024 19:30
@Masara
Copy link
Contributor Author

Masara commented May 3, 2024

@lars-reimann Should be fixed now, could you take another look?

@lars-reimann
Copy link
Member

lars-reimann commented May 3, 2024

Code is being generated again, and the imports look good as well.

I greatly preferred the previous folder structure, though. Previously, there were folders for the package parts (e.g. safeds/data/tabular/containers) and then individual files inside them (e.g. table.sdsstub, column.sdsstub, ...). Now the files are combined into one, leading to very long stub files like safeds/data/tabular/containers.sdsstub). The point of having packages is that more than one file can belong to them.

The behavior from #81 should be restored. If it's easier, I'd also be fine with generating one file for each global declaration (class, global function, ...) with the same name as the declaration.

@Masara
Copy link
Contributor Author

Masara commented May 3, 2024

@lars-reimann How do you mean should it look like? Instead of writing every declaration into, for example, the safeds/data/tabular/containers.sdsstub file, I should create files like safeds/data/tabular/containers/Column.sdsstubs which only contains one class / global function?

@lars-reimann
Copy link
Member

@lars-reimann How do you mean should it look like? Instead of writing every declaration into, for example, the safeds/data/tabular/containers.sdsstub file, I should create files like safeds/data/tabular/containers/Column.sdsstubs which only contains one class / global function?

That would be OK, yes.

@Masara
Copy link
Contributor Author

Masara commented May 4, 2024

I changed the way the directories are build, is this all right now?

@lars-reimann
Copy link
Member

lars-reimann commented May 4, 2024

The file structure is great now. However, now all packages are wrong again. Example (safeds/data/tabular/containers/Column.sdsstub):

Actual:

package safeds.data.tabular.containers.Column

Expected:

package safeds.data.tabular.containers

Another issue is that files import their own declaration. Example (safeds/data/tabular/containers/Column.sdsstub):

from safeds.data.tabular.containers import Column

These imports should be removed.

@lars-reimann
Copy link
Member

lars-reimann commented May 4, 2024

We also need tests to detect these regressions or look more closely at updated snapshots before overwriting them.

@Masara Masara force-pushed the 82-wrong-imports-in-created-stubs branch from cd98e08 to 4a0db67 Compare May 4, 2024 10:56
@Masara
Copy link
Contributor Author

Masara commented May 4, 2024

That was on me, I didn't check the snapshots thoroughly last time. It should work correctly now.

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.

Awesome, works very well.

@lars-reimann lars-reimann merged commit 48c5367 into main May 4, 2024
8 checks passed
@lars-reimann lars-reimann deleted the 82-wrong-imports-in-created-stubs branch May 4, 2024 12:18
lars-reimann pushed a commit that referenced this pull request May 4, 2024
## [0.3.0](v0.2.0...v0.3.0) (2024-05-04)

### Features

* Added handling for sequence classes ([#127](#127)) ([cb061ab](cb061ab)), closes [#126](#126)
* DocString result names for Safe-DS stub results ([#101](#101)) ([fe163e3](fe163e3)), closes [#100](#100)
* Examples from docstrings are also taken over to stub docstrings ([#116](#116)) ([6665186](6665186)), closes [#115](#115)
* Replace the docstring_parser library with Griffe ([#79](#79)) ([9b2f802](9b2f802))

### Bug Fixes

* `Self` types as results are translated to class names  ([#110](#110)) ([4554a56](4554a56)), closes [#86](#86)
* Creating stubs with relative paths for source and output directories ([#128](#128)) ([b4493c9](b4493c9)), closes [#125](#125)
* Docstrings have the correct indentation for nested classes (stubs) ([#114](#114)) ([c7b8550](c7b8550)), closes [#113](#113)
* Fixed a bug where double ? would be generated for stubs ([#103](#103)) ([c35c6ac](c35c6ac)), closes [#87](#87) [#87](#87)
* Fixed a bug where imports would not check reexports for shortest path ([#112](#112)) ([48c5367](48c5367)), closes [#82](#82)
* Fixed a bug where results in stubs would not be named ([#131](#131)) ([4408c84](4408c84)), closes [#100](#100)
* Fixed a bug which prevented mypy version update ([#107](#107)) ([501d2cd](501d2cd))
* Fixed the stubs generator ([#108](#108)) ([9ad6df6](9ad6df6)), closes [#80](#80)
* Generated names of callback results start with result, not with param ([#104](#104)) ([6e696e9](6e696e9)), closes [#85](#85)
* Include lines of examples that start with `...` ([#130](#130)) ([3477b4a](3477b4a)), closes [#129](#129)
* No "// TODO ..." if return type is explicitly `None` ([#111](#111)) ([08e345f](08e345f)), closes [#83](#83)
* Removed the Epydoc parser ([#89](#89)) ([684a101](684a101))
* Replaced tabs with 4 spaces ([#105](#105)) ([8e7aa5d](8e7aa5d)), closes [#84](#84)
* The file structure of stubs resembles the "package" path. ([#106](#106)) ([ff1800e](ff1800e)), closes [#81](#81)
* Translation of callable ([#102](#102)) ([c581e6a](c581e6a)), closes [#88](#88) [#88](#88)
@lars-reimann
Copy link
Member

🎉 This PR is included in version 0.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lars-reimann lars-reimann added the released Included in a release label May 4, 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.

Wrong imports in created stubs
3 participants