Skip to content

Improve Flake Rules #1214

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 12 commits into from
Feb 8, 2021
Merged

Improve Flake Rules #1214

merged 12 commits into from
Feb 8, 2021

Conversation

krishnakalyan3
Copy link
Contributor

Followup of #1172

@krishnakalyan3
Copy link
Contributor Author

Import files that need to be replaced by all

test/torchaudio_unittest/common_utils/__init__.py
torchaudio/backend/__init__.py
torchaudio/functional/__init__.py
torchaudio/sox_effects/__init__.py
torchaudio/models/__init__.py
torchaudio/__init__.py

@krishnakalyan3
Copy link
Contributor Author

krishnakalyan3 commented Jan 31, 2021

@mthrok we have two options according to

from .my_class import MyClass

__all__ = ['MyClass',]

or

from .my_class import MyClass  # noqa: F401

Confirming that the first option was preferred right?

@mthrok
Copy link
Collaborator

mthrok commented Feb 1, 2021

@mthrok we have two options according to

from .my_class import MyClass

__all__ = ['MyClass',]

or

from .my_class import MyClass  # noqa: F401

Confirming that the first option was preferred right?

Hi @krishnakalyan3

Thanks for the follow up. Yes, the __all__ is the preferred approach for public APIs, but for internal APIs, noqa is preferred. So, for backend module, let's use noqa.

Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

@krishnakalyan3 Thank you for working on this. It looks good. Is this ready to merge? (the failing I/O tests are flaky ones that I am hunting down the condition, so we can ignore.)

@krishnakalyan3
Copy link
Contributor Author

krishnakalyan3 commented Feb 8, 2021

@mthrok yes.

@mthrok mthrok merged commit 0f23e6d into pytorch:master Feb 8, 2021
@mthrok
Copy link
Collaborator

mthrok commented Feb 8, 2021

Thanks!

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.

3 participants