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 linters for Docker, Go, Python, and Shell #57

Merged
merged 17 commits into from
Oct 25, 2023
Merged

Conversation

Jeffrey-Vervoort-KNMI
Copy link
Collaborator

@Jeffrey-Vervoort-KNMI Jeffrey-Vervoort-KNMI commented Oct 20, 2023

First, the refactoring of the project needs to be merged (See this branch: https://github.com/EURODEO/e-soh-datastore-poc/tree/generalize_putobservations), and then this PR can be merged. Done

This PR contains:

  • Adds linters and formatters for:
    • Docker
    • Go
    • Python
    • Shell
  • Adds a github actions for the linters and formatters.
  • Adds a pre-commit hook with the linters and formatters.
  • Updates the readme on how to install the pre-commit hook.
  • Applied the pre-commit hook to all of the files.

@Jeffrey-Vervoort-KNMI
Copy link
Collaborator Author

First, I think it's great to have these linters in place! I really only have a general comment: how does the pre-commit hook work (in file .pre-commit-config.yaml) ? Is source code modified to conform to certain styles? While that's not a problem in itself, it might be challenging if your IDE/editor (vscode in my case) is configured to modify source file according to a different standard upon file save. I.e. the diffs may in certain cases contain a lot of "noise" that will make it harder to distinguish essential changes from automatically applied style changes. But I'm not sure it will work that way in practice, so I'm only guessing here :)

Rebasing was easier on a new branch. Moved Jo's comment from to here and closed the old MR.

@Jeffrey-Vervoort-KNMI
Copy link
Collaborator Author

How does the pre-commit hook work (in file .pre-commit-config.yaml) ?

I have updated the README.md in the root of the repository and added the steps to install and work with the pre-commit hook.

Is source code modified to conform to certain styles? While that's not a problem in itself, it might be challenging if your IDE/editor (vscode in my case) is configured to modify source file according to a different standard upon file save.

For me, it is the reason why I favour linters and formatters. Using them enables anyone to pick the editor they like. Because the tool is then responsible for the formatting and not the editor. Someone can then use VSCode, Intelij or even notepad.

I.e. the diffs may in certain cases contain a lot of "noise" that will make it harder to distinguish essential changes from automatically applied style changes.

The first time - which is in this MR - there is a lot of noise, thereafter the formatting should be the same for everyone. This should minimise the diffs.

@Jeffrey-Vervoort-KNMI Jeffrey-Vervoort-KNMI changed the title Add linters forPython, Go and Docker Add linters for Docker, Go, Python, and Shell Oct 23, 2023
@lukas-phaf
Copy link
Contributor

I think what Jo is worried about is formatting "ping-pong". The editor saves with "format A", the pre-commit hooks run, reformat the code to "format B", the editor puts it back to "format A", etc.

But maybe we should just try it? As this MR adds only a single space to the Go code, and we have no issues with this in Python (although not sure if anybody uses vscode in our group) with these linters.
We can also back out if we see problems.

README.md Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
tstester/config.json Outdated Show resolved Hide resolved
@Jeffrey-Vervoort-KNMI Jeffrey-Vervoort-KNMI merged commit 366b60f into main Oct 25, 2023
2 checks passed
@Jeffrey-Vervoort-KNMI Jeffrey-Vervoort-KNMI deleted the ci-cd-2 branch October 25, 2023 07:10
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