Skip to content

Conversation

@vivekmig
Copy link
Contributor

@vivekmig vivekmig commented Mar 3, 2020

This applies isort to alphabetize import ordering. We also add an isort check to CircleCI to ensure that import ordering is maintained in future PRs. This required adding an isort config that is compatible with black, which was found here: https://github.com/psf/black

This also adds a type: ignore for an issue caused by merging Insights type fixes with type hints for common.

@vivekmig vivekmig requested review from NarineK and edward-io March 3, 2020 00:10
for inputs, additional_forward_args, label in _batched_generator(
# Type ignore for issue with passing union to function taking generic
# https://github.com/python/mypy/issues/1533
for inputs, additional_args, label in _batched_generator( # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes were necessary to fix mypy failures on master due to merging with the common type hints. additional_forward_args was renamed to additional_args since the "type: ignore" made the line too long causing flake8 errors.
CC: @edward-io

Copy link
Contributor

Choose a reason for hiding this comment

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

could we do:

for (
  inputs,
  additional_forward_args,
  label,
) in _batched_generator( # type: ignore

instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I figured a single line is more concise, but this works too, will update, thanks!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@vivekmig has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@edward-io edward-io left a comment

Choose a reason for hiding this comment

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

Looks good to me, though I didn't review all 86 files. Minor comments, LGTM.

for inputs, additional_forward_args, label in _batched_generator(
# Type ignore for issue with passing union to function taking generic
# https://github.com/python/mypy/issues/1533
for inputs, additional_args, label in _batched_generator( # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

could we do:

for (
  inputs,
  additional_forward_args,
  label,
) in _batched_generator( # type: ignore

instead?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@vivekmig has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@vivekmig has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@vivekmig merged this pull request in 37555ab.

NarineK pushed a commit to NarineK/captum-1 that referenced this pull request Nov 19, 2020
Summary:
This applies isort to alphabetize import ordering. We also add an isort check to CircleCI to ensure that import ordering is maintained in future PRs. This required adding an isort config that is compatible with black, which was found here: https://github.com/psf/black

This also adds a type: ignore for an issue caused by merging Insights type fixes with type hints for common.
Pull Request resolved: meta-pytorch#310

Reviewed By: orionr

Differential Revision: D20206324

Pulled By: vivekmig

fbshipit-source-id: 48cce82d60a9a6f7384211e7aae2ffb4c0c85772
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants