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

Add automatic formatting and linting #1571

Merged
merged 12 commits into from Apr 24, 2023

Conversation

matthewhegarty
Copy link
Contributor

@matthewhegarty matthewhegarty commented Apr 13, 2023

Problem

We have a long-standing issue in that we don't follow best practices by not having automatic linting / formatting. This PR adds rules to apply black formatting and flake8 checks.

Solution

  • Added .pre-commit-config.yaml to include rules for formatting using black, flake8 and isort
  • pre-commit checks should automatically be run for PRs using pre-commit.ci
  • removed isort from tox: it is now redundant and replaced by pre-commit hooks
  • Updated documentation on how to install pre-commit
  • Added .editorconfig

I will apply global reformatting in a separate PR.

  • Test linting works on PR submit

Acceptance Criteria

  • Have ran tests

@coveralls
Copy link

coveralls commented Apr 13, 2023

Coverage Status

Coverage: 100.0%. Remained the same when pulling 4718b85 on matthewhegarty:add-precommit into 8c9a5ef on django-import-export:main.

@matthewhegarty matthewhegarty marked this pull request as ready for review April 14, 2023 12:07
Copy link

@pokken-magic pokken-magic left a comment

Choose a reason for hiding this comment

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

Wow, thanks for doing this Matthew! I think this looks solid to me.

Copy link

@pokken-magic pokken-magic left a comment

Choose a reason for hiding this comment

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

I am overall fine with the defaults I think, but I will say screen size has been seriously increasing and there's no real functional problem with a more modern line length. I've used 120 and it's really nice in some ways. So I'd be open to that too. If you have strong opinions though the 88 default is fine.

@matthewhegarty
Copy link
Contributor Author

Hi Ryan - thanks for reviewing. I likewise don't have strong opinions on line length, however I thought it best to be as consistent as possible, so went with 88 which is the black default and used by other projects.

@matthewhegarty matthewhegarty merged commit 27e868c into django-import-export:main Apr 24, 2023
12 checks passed
@matthewhegarty
Copy link
Contributor Author

@pokken-magic Hi Ryan, we are having to make some changes to the repo to support GitHub Sponsors. One change is that you will need 2FA setup on Github. Hence that is why you were removed and re-invited. If you want to contact me I can explain further.

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.

None yet

3 participants