Skip to content

Conversation

Popkornium18
Copy link
Contributor

The previous code did not export the Sqids class leading to private import usage warnings when using from sqids import Sqids. One could work around it by using from sqids.sqids import Sqids. This workaround is now no longer necessary.

@Pevtrick
Copy link
Collaborator

Hey thanks for the PR, i wasn't aware that this could result in a warning. I'd like to read more about it, would you mind providing resources related to the warning? Does __all__ = ['Sqids'] fix your problem as well? If so, i think it is a better solution as one won't oversee it in case of additional package level "exports" in the future.

@Popkornium18
Copy link
Contributor Author

Popkornium18 commented Mar 10, 2025

You can reproduce the issue like this. This is file.py

from sqids import Sqids
_ = Sqids()

Then run:

$ mypy --strict file.py
file.py:1: error: Module "sqids" does not explicitly export attribute "Sqids"  [attr-defined]
$ pyright file.py
/home/user/file.py
  /home/user/file.py:1:19 - error: "Sqids" is not exported from module "sqids"
    Import from "sqids.sqids" instead (reportPrivateImportUsage)
1 error, 0 warnings, 0 informations

__all__ = ["Sqids"] does also solve the issue. I just prefer the from sqids import Sqids as Sqids way, since it looks better in a file with tons of exports and it is simpler to append another export and keep them sorted with reorder-python-imports/isort. But __all__ = ["Sqids"] is probably more common. Something about that String notation just rubs me the wrong way.

But I can change it to __all__ if you prefer.

@Pevtrick
Copy link
Collaborator

Okay i see it is a mypy warning... that is why i didn't see it in my test setup. Yes please change it to the __all__ notation. Thx for clarification. 😊

The previous code did not export the Sqids class leading to private
import usage warnings when using `from sqids import Sqids`.
One could work around it by using `from sqids.sqids import Sqids`.
This workaround is now no longer necessary.
@Popkornium18
Copy link
Contributor Author

done

@Pevtrick
Copy link
Collaborator

I will take a look at this tomorrow... 😅

@Pevtrick Pevtrick merged commit 2741b75 into sqids:main Mar 26, 2025
8 checks passed
@jules-ch
Copy link

Possible to release a new version to correct this ? :)

@4kimov
Copy link
Member

4kimov commented May 13, 2025

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants