Skip to content
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

Format all code in Gammapy black #1764

Merged
merged 9 commits into from Sep 6, 2018
Merged

Format all code in Gammapy black #1764

merged 9 commits into from Sep 6, 2018

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Sep 6, 2018

This PR runs make black to format all code in Gammapy black (see #1423).

This is a safe transform, the resulting code will execute identically as before always. The only question here is about code style.

Personally I like most of the changes, but there's also quite a few where I think there's too many line breaks. But overall I'm +1 to do this now and then keep the code black, because then we never have to think or discuss about code formatting, that question is removed. (and when we get contributions, we don't even have to review / discuss code style, we just ask people to run make black before making a PR or do it ourselves).

@adonath @registerrier @joleroi - Thoughts?

@cdeil cdeil added the cleanup label Sep 6, 2018
@cdeil cdeil added this to the 0.8 milestone Sep 6, 2018
@cdeil cdeil self-assigned this Sep 6, 2018
spec.fill(self.energy) # leaving spec.fill(self) was triggering an issue for the LAT event list
spec.fill(
self.energy
) # leaving spec.fill(self) was triggering an issue for the LAT event list

This comment has been minimized.

@cdeil

cdeil Sep 6, 2018
Author Member

This is an example where the formatting isn't great and manual formatting would improve.
If the comment is moved on the line before, the code will fit on one line.

('observations', 'check_observations'),
('consistency', 'check_consistency'),
]
)

This comment has been minimized.

@cdeil

cdeil Sep 6, 2018
Author Member

This is a case where I think black is putting too many lines and the previous version was better.
But this is not easy to change in black, because it just breaks on the outermost parenthesis, and doing a better job would require a more complex line break algorithm that looks deeper at the content inside parentheses. So this is something I don't like, but OK, I can live with it to get the benefits from using black.

@adonath
Copy link
Member

@adonath adonath commented Sep 6, 2018

I agree the results are good and I think we should make the transition to use black for the code formatting. The resulting consistent code style helps a lot to read the code and also helps to simpify the review process for new contributions. The remaining cases where the black result is not that nice are either at least acceptable or, as you already noted, can be sorted out by hand later.

If we decide to make this change, we should of course add a new section on the developer's how to docs page.

@Bultako
Copy link
Member

@Bultako Bultako commented Sep 6, 2018

👍 to make the transition to use black

@cdeil
Copy link
Member Author

@cdeil cdeil commented Sep 6, 2018

OK, I'll do some cleanup and then merge this tonight.

FYI: I did file one issue with black mentioning my gripe with the output it produces:
psf/black#500

@cdeil
Copy link
Member Author

@cdeil cdeil commented Sep 6, 2018

I spent a bit of time to manually improve code formatting based on the diff here. But I only got through ~ a third I think and as discussed above this work can be continued later.

I'll do one more commit where I remove the --skip-string-normalization option from black, and then merge as-is.

@cdeil cdeil merged commit f6fe37d into gammapy:master Sep 6, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil cdeil mentioned this pull request Sep 6, 2018
@cdeil cdeil changed the title Run make black to format all code in Gammapy black Format all code in Gammapy black Sep 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants