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

Use black for code formatting #191

Merged
merged 6 commits into from
May 8, 2019
Merged

Use black for code formatting #191

merged 6 commits into from
May 8, 2019

Conversation

basnijholt
Copy link
Member

No description provided.

@akhmerov
Copy link
Contributor

akhmerov commented May 6, 2019

Coincidentally I also checked black recently. It's a decent idea. Sometimes its default formatting is not very good (for instance I stumbled upon psf/black#824), but the advantage of never needing to bother with minutia could outweigh this drawback.

@akhmerov
Copy link
Contributor

akhmerov commented May 6, 2019

Is it possible to separate black and other linters from the rest of the pipeline?

@basnijholt
Copy link
Member Author

We can create a separate lint-pipeline.yml but why would we do that?

@akhmerov
Copy link
Contributor

akhmerov commented May 6, 2019

We can create a separate lint-pipeline.yml but why would we do that?

So that it is clearer for people submitting the code what goes wrong. Also so that we may ignore it or apply it on our own.

Also if we're using black, we need to document it and make a pre-commit hook.

@basnijholt
Copy link
Member Author

We could use https://github.com/pre-commit/pre-commit for that.

@basnijholt
Copy link
Member Author

So that it is clearer for people submitting the code what goes wrong. Also so that we may ignore it or apply it on our own.

But if the tests fail one can just click on the details and see what failed exactly, which is what one needs to do if something fails anyway.

@akhmerov
Copy link
Contributor

akhmerov commented May 6, 2019

Well, for example right now the authors check failed but we don't see that.

@akhmerov
Copy link
Contributor

akhmerov commented May 6, 2019

pre-commit seems OK (if only a bit of an overkill for a pure python package with one hook so far), but my point is mainly that we should document how to use black in e.g. CONTRIBUTING.md

@basnijholt
Copy link
Member Author

A relevant issue in black psf/black#148.

@basnijholt basnijholt changed the base branch from master to stable-0.8 May 7, 2019 23:47
@basnijholt basnijholt force-pushed the black branch 3 times, most recently from 03f5a3b to 1e2ac8f Compare May 8, 2019 00:26
@basnijholt
Copy link
Member Author

@akhmerov I've added pre-commit with flake8, black, and white-space fixers.

I've fixed all flake8 issues too and added this to the "Development" section https://github.com/python-adaptive/adaptive/pull/191/files#diff-88b99bb28683bd5b7e3a204826ead112.

rev: 3.7.4
hooks:
- id: flake8
args: ['--max-line-length=500', '--ignore=E203,E266,E501,W503', '--max-complexity=18', '--select=B,C,E,F,W,T4,B9']
Copy link
Member Author

Choose a reason for hiding this comment

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

These settings are like this to make black compatible with flake8

@basnijholt basnijholt force-pushed the black branch 11 times, most recently from f7d3afa to 62232b0 Compare May 8, 2019 18:13
@akhmerov
Copy link
Contributor

akhmerov commented May 8, 2019

LGTM

@basnijholt basnijholt merged commit edc36d9 into stable-0.8 May 8, 2019
@basnijholt
Copy link
Member Author

I am merging into stable to avoid many merge conflicts in the future.

@basnijholt basnijholt deleted the black branch May 8, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants