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 more pre-commit linting #518

Merged

Conversation

jorisvandenbossche
Copy link
Member

The pre-commit linting related commits by @pradyunsg from #514, so those can already be merged separately

These make it much easier to not have minor nits across various
files in the repository.
Prettier provides the same experience as Python's Black, when working on
web-related stuff.
@jorisvandenbossche
Copy link
Member Author

@choldgraf I am personally fine with those, you're OK with merging? (as long as pre-commit runs them, I don't care too much which exact linting rules we use :))

@choldgraf
Copy link
Collaborator

+1 from me, any objection to using pre-commit.ci ?

@jorisvandenbossche jorisvandenbossche merged commit 41764f5 into pydata:master Nov 22, 2021
@jorisvandenbossche jorisvandenbossche deleted the precommit-linting branch November 22, 2021 21:31
@jorisvandenbossche
Copy link
Member Author

Thanks @pradyunsg !

(enabled "rebase and merge" for a moment, to preserve Pradyun's commits)

any objection to using pre-commit.ci ?

I have no experience with it. What does it exactly provide over the current pre-commit running in github actions? Automatic fixing?
(I am not sure if I am in favor of by default running this automatically, IMO it can also give annoying conflicts if commits were added remotely that you don't have locally, and fixing that up is not necessarily a newbie git workflow. I would personally prefer an "on demand" automatic fix)

@choldgraf
Copy link
Collaborator

yeah - the main benefit I've found is when people make edits to things directly from the GitHub UI, or where they make edits before remembering to install pre-commit locally. In those cases, if you forgot to do something that pre-commit would have fixed, it is automatically applied to the PR. So most useful for first time contributors or drive-by contributors. Not a huge deal either way but maybe something we can consider if we run into this issue relatively often

@pradyunsg
Copy link
Contributor

FWIW, you can't use pre-commit.ci unless you drop the "local" step that builds things using the yarn-installed environment. :)

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.

3 participants