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

pre-commit #129

Merged
merged 4 commits into from
Jun 17, 2024
Merged

pre-commit #129

merged 4 commits into from
Jun 17, 2024

Conversation

PLangowski
Copy link
Collaborator

No description provided.

@PLangowski PLangowski force-pushed the pre-commit branch 4 times, most recently from 020a54c to 8d875dd Compare June 6, 2024 09:37
@PLangowski PLangowski changed the title WIP: pre-commit pre-commit Jun 6, 2024
@PLangowski PLangowski requested a review from macpijan June 6, 2024 09:50
@PLangowski PLangowski force-pushed the pre-commit branch 3 times, most recently from 80e7b4a to ad6b4fb Compare June 7, 2024 08:10
@DaniilKl DaniilKl requested review from DaniilKl and removed request for macpijan June 7, 2024 09:02
@PLangowski PLangowski force-pushed the pre-commit branch 2 times, most recently from 13589e1 to 351e05e Compare June 11, 2024 10:10
@PLangowski
Copy link
Collaborator Author

@DaniilKl @macpijan I added the fix flag so that oelint fixes some of the files (doesn't work on all checks).
I also added a custom rule that will check for multiline variable assignment and require a 4-space indent to be present. 3b03cfe This, however, does not work yet. I will try to work this out.

@PLangowski
Copy link
Collaborator Author

I'm getting the following error

Can't load rule .oelint-rules.assignment_indent_rule -> the 'package' argument is required to perform a relative import for '.oelint-rules.assignment_indent_rule'

@PLangowski
Copy link
Collaborator Author

The rule file could not be loaded because it was in a hidden directory and Python couldn't find it.

@PLangowski
Copy link
Collaborator Author

@macpijan I got my custom rule to work. It turns out that some of the existing recipes comply to the original oelint rule - They indent to the " character in the first line:

SRC_URI = "git://github.com/nix-community/acpi_call.git;branch=master;protocol=https \
           file://0001-Makefile-fixing-build-errrors.patch \
           file://0001-GPLv3-licence-file-added.patch \
           "

We need to decide what we want: Always a 4 space indent or indenting up to "? What do you think?

@macpijan
Copy link
Contributor

macpijan commented Jun 12, 2024

We need to decide what we want: Always a 4 space indent or indenting up to "? What do you think?

Which rule is that? This one as linked in the logs here: #129 (comment)

Can it be autofixed by the tool?

I have been using so far the 4 spaces, but there may be some reason why the upstream linter tool uses the other approach.

It was enough for 3mins search to find this: https://docs.yoctoproject.org/dev/contributor-guide/recipe-style-guide.html#variable-formatting

This should close the discussion and our preference does not matter much. We want to keep the style as in the upstream recipes, to have easier time of upstreaming them and keep the good practices as suggested by the project.

.oelint-ruleset.json Outdated Show resolved Hide resolved
@PLangowski
Copy link
Collaborator Author

Unfortunately, a lot of the rules, including the indent, do not have an autofix and need to be changed manually

@PLangowski
Copy link
Collaborator Author

@macpijan I managed to create an autofix for the indent check. I seems to work except for one case: When the first line of the variable alrerady has something other than "\, e.g.:

SRC_URI = "file://some-file \
<other files>

I haven't managed to get the metod to fix that.

@macpijan
Copy link
Contributor

Sounds like a good start @PLangowski , nice work.

@PLangowski PLangowski force-pushed the pre-commit branch 4 times, most recently from f317164 to 91d10f8 Compare June 17, 2024 09:25
@macpijan
Copy link
Contributor

One more: let's try to cleanup commits.

Signed-off-by: Pawel Langowski <pawel.langowski@3mdeb.com>
The script was moved to the
dts-scripts repo
(https://github.com/Dasharo/dts-scripts/blob/main/include/dts-functions.sh)
and is not used here anymore.

Signed-off-by: Pawel Langowski <pawel.langowski@3mdeb.com>
…-body` flag

Signed-off-by: Pawel Langowski <pawel.langowski@3mdeb.com>
Signed-off-by: Pawel Langowski <pawel.langowski@3mdeb.com>
@PLangowski
Copy link
Collaborator Author

I cleaned up the commits

@DaniilKl
Copy link
Contributor

@PLangowski, ping me please when review will be needed and use draft for a PR next time to signal when the PR is not ready for review.

@PLangowski
Copy link
Collaborator Author

@DaniilKl it is ready for review now.

@macpijan
Copy link
Contributor

Good work overall @PLangowski I have very high hopes on overall quality increase because of these changes.

@macpijan macpijan merged commit 22840b7 into develop Jun 17, 2024
@macpijan macpijan deleted the pre-commit branch June 17, 2024 18:06
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.

4 participants