Skip to content

formatting fixed #2372

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 20, 2021
Merged

formatting fixed #2372

merged 7 commits into from
Dec 20, 2021

Conversation

@louis-she
Copy link
Contributor

@louis-she louis-she commented Dec 20, 2021 •
edited by vfdev-5
Loading

Related to #2366

Description:

Removed all the tailing comma that will mislead the black

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added ci CI docker examples Examples module: contrib Contrib module module: distributed Distributed module module: engine Engine module module: handlers Core Handlers module module: metrics Metrics module module: utils Utils module labels Dec 20, 2021
@louis-she louis-she changed the base branch from master to black-21.12b0 December 20, 2021 06:52
vfdev-5
vfdev-5 reviewed Dec 20, 2021
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot for the PR @louis-she !
I left few comments where we still need to remove trailing commas and rerun black.
Otherwise, you almost covered everything and that's very helpful to quickly finish the issue, great job !

EDIT:
There are also some line-length issues detected by flake8:

ignite/engine/engine.py:449:121: E501 line too long (121 > 120 characters)
ignite/distributed/utils.py:152:121: E501 line too long (121 > 120 characters)

let's fix them too

@louis-she
Copy link
Contributor Author

louis-she commented Dec 20, 2021

@vfdev-5 comments were fixed, and changed all the black version to 21.12b0

vfdev-5
vfdev-5 approved these changes Dec 20, 2021
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @louis-she !

vfdev-5
vfdev-5 reviewed Dec 20, 2021
View reviewed changes
tests/ignite/test_utils.py Outdated
"""Docs are cool"""
return 24

assert func_no_reasons.__doc__ == "**Deprecated function**.\n\n Docs are cool\n .. deprecated:: 0.4.2"
Copy link
Collaborator

@vfdev-5 vfdev-5 Dec 20, 2021

Choose a reason for hiding this comment

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

Tests are onw failing here as \n is removed in the docstring of func_no_reasons. Let's update expected result:

Suggested change
assert func_no_reasons.__doc__ == "**Deprecated function**.\n\n Docs are cool\n .. deprecated:: 0.4.2"
assert func_no_reasons.__doc__ == "**Deprecated function**.\n\n Docs are cool .. deprecated:: 0.4.2"

Copy link
Collaborator

@vfdev-5 vfdev-5 Dec 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@louis-she below expected results are needed to be updated as well, but I'm not 100% sure if just removing "\n" is enough...

Can you try to execute this test locally :

pytest -vvv tests/ignite/test_utils.py::test_deprecated

vfdev-5
vfdev-5 reviewed Dec 20, 2021
View reviewed changes
@louis-she
Copy link
Contributor Author

louis-she commented Dec 20, 2021

@vfdev-5 fixed, we could trigger the CI again.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 20, 2021

@vfdev-5 fixed, we could trigger the CI again.

I think other expected docs will fail, see my comment : #2372 (comment)

@louis-she
Copy link
Contributor Author

louis-she commented Dec 20, 2021 •
edited
Loading

Turns out there are some spaces that should also be removed. The test_deprecated is passed now. But if I run all tests locally, some of the tests of visdom failed due to network connection. Not sure if it is environment related issue.

image

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 20, 2021

Turns out there are some spaces that should also be removed. The test_deprecated is passed now. But if I run all tests locally, some of the tests of visdom failed due to network connection. Not sure if it is environment related issue.

image

Thanks! I rerun the tests and no worries about visdom it won't pass without its server setup.

@vfdev-5 vfdev-5 merged commit 1bc72ca into pytorch:black-21.12b0 Dec 20, 2021
@louis-she louis-she deleted the black branch December 20, 2021 23:55
vfdev-5 added a commit that referenced this pull request Dec 21, 2021
@louis-she @vfdev-5
Chenglu/black (#2378)
5737b06 
* Updated black to 21.12b0 and fixed codebase for others folders (#2367)

* Updated black to 21.12b0 and fixed codebase for others folders

* Updated black

* formatting fixed (#2372)

* fix: remove tailing commas that would mislead black formating

* autopep8 fix

* fix: left out tailing comma

* chg: change black version

* fix: test_deprecated assert failed with additional newline

* fix: typo

* fix: fix func_no_reasons test assert failed

Co-authored-by: louis-she <louis-she@users.noreply.github.com>

Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: louis-she <louis-she@users.noreply.github.com>
Ishan-Kumar2 pushed a commit to Ishan-Kumar2/ignite that referenced this pull request Dec 26, 2021
@louis-she @vfdev-5 @Ishan-Kumar2
Chenglu/black (pytorch#2378)
5dcee96 
* Updated black to 21.12b0 and fixed codebase for others folders (pytorch#2367)

* Updated black to 21.12b0 and fixed codebase for others folders

* Updated black

* formatting fixed (pytorch#2372)

* fix: remove tailing commas that would mislead black formating

* autopep8 fix

* fix: left out tailing comma

* chg: change black version

* fix: test_deprecated assert failed with additional newline

* fix: typo

* fix: fix func_no_reasons test assert failed

Co-authored-by: louis-she <louis-she@users.noreply.github.com>

Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: louis-she <louis-she@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vfdev-5 vfdev-5 vfdev-5 approved these changes

Assignees

No one assigned

Labels

ci CI docker examples Examples module: contrib Contrib module module: distributed Distributed module module: engine Engine module module: handlers Core Handlers module module: metrics Metrics module module: utils Utils module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update black to 21.12b0

2 participants

@louis-she @vfdev-5