Skip to content

make type alias private #7266

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

Merged
merged 2 commits into from
Feb 16, 2023
Merged

make type alias private #7266

merged 2 commits into from
Feb 16, 2023

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Feb 16, 2023

Since they are just used internally, there is no need to have them public.

cc @vfdev-5 @bjuncek

Comment on lines +2 to +5
from ._datapoint import _FillType, _FillTypeJIT, _InputType, _InputTypeJIT
from ._image import _ImageType, _ImageTypeJIT, _TensorImageType, _TensorImageTypeJIT, Image
from ._mask import Mask
from ._video import TensorVideoType, TensorVideoTypeJIT, Video, VideoType, VideoTypeJIT
from ._video import _TensorVideoType, _TensorVideoTypeJIT, _VideoType, _VideoTypeJIT, Video
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the change with the smallest diff. We could also remove them from this namespace completely and just import from the respective module directly where we need them. If we do that, should we still prefix them with an underscore? Just removing them here would also mark them private, since the module there are defined in is private as well.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 16, 2023

Do you see any harm of exposing them as public types ?

@pmeier
Copy link
Collaborator Author

pmeier commented Feb 16, 2023

Do you see any harm of exposing them as public types ?

Yes. If we make them public, we need to keep BC. Meaning if something changes, for example #7252 we have to work around that.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 16, 2023

OK, why not, but those are just types and we are not exposing py.typed so there is little chances they are used, IMO

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Stamping, I don't have a super strong opinion other than erring on the side of keeping such ambiguous things private, until users ask for them to be public.

@pmeier pmeier merged commit d4d20f0 into pytorch:main Feb 16, 2023
@pmeier pmeier deleted the private-types branch February 16, 2023 17:47
@NicolasHug NicolasHug mentioned this pull request Feb 17, 2023
49 tasks
facebook-github-bot pushed a commit that referenced this pull request Mar 28, 2023
Reviewed By: vmoens

Differential Revision: D44416583

fbshipit-source-id: 33f61d6cae4f1ac19b68376b8982069135353eef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants