Skip to content

[DO NOT MERGE] introduce isort and black as autoformatters #4178

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

Closed
wants to merge 15 commits into from

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Jul 14, 2021

Closes #4174. For now this is just a proof of concept PR. DO NOT MERGE this until we actually want to use black and isort.

Only pyproject.toml and setup.cfg need review. All the other changes were made by the autoformatters or to accommodate flake8.

Todo:

  • Add black and isort to CI
  • Add black and isort as pre-commit hooks

from torchvision.ops.boxes import box_area

from typing import Optional, List, Dict, Tuple, Union
from .roi_align import roi_align
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Our current implementation relies on a specific import order in torchvision/ops/__init__.py since we have torchvision/ops/roi_align.py as well as a roi_align function. Thus, depending on the current stage of the imports from torchvision.ops import roi_align gives you either the module or the function.

In the future we should avoid name conflicts between modules and functions. For example, we could rename the module to _roi_align.py since all its functionality is available through torchvision.ops.

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.

Thanks @pmeier !

Considering the sequence of changes that will need to be done (in particular the doc updates, the pre-commit hook and the CI check), I would suggest to do all this in a separate branch first. We'll then be able to merge everything in master in one go, while keeping the development in separate PRs without worrying too much about the current state of master.

@pmeier pmeier mentioned this pull request Jul 15, 2021
@pmeier
Copy link
Collaborator Author

pmeier commented Sep 8, 2021

Superseded by #4384.

@pmeier pmeier closed this Sep 8, 2021
@pmeier pmeier deleted the black-isort branch September 8, 2021 15:53
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.

Use black?
3 participants