Skip to content

Add static checks#218

Merged
tstromberg merged 2 commits into
google:masterfrom
mik-laj:static-checks
Jan 15, 2021
Merged

Add static checks#218
tstromberg merged 2 commits into
google:masterfrom
mik-laj:static-checks

Conversation

@mik-laj
Copy link
Copy Markdown

@mik-laj mik-laj commented Oct 7, 2020

Code quality is important, especially in an open-source project where the code will be maintained for a long time and by multiple people. So I would like to suggest using https://pre-commit.com framework to automate a lot of stuff and assure that our code is nice.

It plays nicely with Github action and allows a plethora of checks (building TOC, license insert, liniting etc) and gives the possibility to build custom ones. For example, here's configuration from Apache Airflow:
https://github.com/apache/airflow/blob/master/.pre-commit-config.yaml

For now I only focused on the basic checks, but in the future, we may add some more Go project-specific checks e.g.golangci-lint or other checks

@mik-laj
Copy link
Copy Markdown
Author

mik-laj commented Oct 7, 2020

@tstromberg WDYT?

@TobKed
Copy link
Copy Markdown

TobKed commented Oct 7, 2020

I have suggestion that for easier review of changes which add linters is to make separate commits for adding linters and parsing project files to distinguish manual and automatic changes.

@mik-laj
Copy link
Copy Markdown
Author

mik-laj commented Oct 7, 2020

@TobKed This is not a very big project. Do you think it's worth the extra work to keep these changes separate?

@TobKed
Copy link
Copy Markdown

TobKed commented Oct 7, 2020

@mik-laj in this case, when it is already done it is probably not worth extra work. However in general, IMHO, it is good practice to keep manual/automatic changes separated 😄

@tstromberg
Copy link
Copy Markdown
Collaborator

I'm not sure how I feel about the TOC changes yet. - but at this point, let's just get this in and sort out the changes later.

@tstromberg tstromberg merged commit 7bbd00b into google:master Jan 15, 2021
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