Skip to content

Black formatting #1934

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 1 commit into from
Apr 17, 2019
Merged

Black formatting #1934

merged 1 commit into from
Apr 17, 2019

Conversation

eshvk
Copy link
Contributor

@eshvk eshvk commented Apr 12, 2019

No description provided.

@eshvk eshvk requested a review from harperj April 15, 2019 20:50
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
@Unity-Technologies Unity-Technologies deleted a comment Apr 15, 2019
Copy link
Contributor

@harperj harperj left a comment

Choose a reason for hiding this comment

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

A few things I'd want to see before merging this:

  • a script to run the exact command you ran to format the codebase
  • addition of checking that black has been run / passes on all PRs added to the circleCI config
  • a descriptive commit message

IMO the changes from the formatter look good, consistency is nice.

Copy link
Contributor

@awjuliani awjuliani left a comment

Choose a reason for hiding this comment

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

Visually it looks great. I agree with Harper however wrt changes needed before we should go ahead with it. We need to ensure that it is a standard we can continue to stick to.

@eshvk eshvk requested a review from harperj April 16, 2019 17:43
@Unity-Technologies Unity-Technologies deleted a comment Apr 16, 2019
@eshvk eshvk assigned eshvk and harperj and unassigned eshvk Apr 16, 2019
@harperj
Copy link
Contributor

harperj commented Apr 16, 2019

Looks good, my only worry is that we don't have any explanation that external contributors need to run black for their PRs and how to do that.

Could be part of https://github.com/Unity-Technologies/ml-agents/blob/master/CONTRIBUTING.md

@eshvk eshvk force-pushed the develop-black branch 2 times, most recently from 488fd6f to f6c9883 Compare April 16, 2019 21:33
Features:
- Reformat code via black.
- Adding circleci configurations.
- Add contribution guidelines.

Steps to reproduce:
- `pip install black`
- `black <source code directory>`
@eshvk
Copy link
Contributor Author

eshvk commented Apr 16, 2019

@harperj fixed PTAL

@Unity-Technologies Unity-Technologies deleted a comment Apr 16, 2019
Copy link
Contributor

@harperj harperj left a comment

Choose a reason for hiding this comment

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

Changes look good, one final thing I'd ask for is some indication somewhere in CONTRIUTING.md or elsewhere that using black is part of the development process.

Otherwise 🚢

@eshvk
Copy link
Contributor Author

eshvk commented Apr 17, 2019

@harperj Can you explain this a little bit more? I added a line in CONTRIBUTING.md asking people to reformat code using black before submitting the PR. I am not sure what else is needed there?

@harperj
Copy link
Contributor

harperj commented Apr 17, 2019

Sorry @eshvk -- I must have missed that among all of the changes. PR looks good to me.

@eshvk eshvk merged commit d54b2d4 into develop Apr 17, 2019
@eshvk eshvk deleted the develop-black branch April 17, 2019 19:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants