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

advisor-result.yml fails yamllint #7219

Closed
dgutson opened this issue Jun 28, 2023 · 4 comments
Closed

advisor-result.yml fails yamllint #7219

dgutson opened this issue Jun 28, 2023 · 4 comments

Comments

@dgutson
Copy link

dgutson commented Jun 28, 2023

yamllint reports indentation and line-length errors on the generated advisor-result.yml file:

  32:5      error    wrong indentation: expected 6 but found 4  (indentation)
  48:7      error    wrong indentation: expected 8 but found 6  (indentation)
  50:5      error    wrong indentation: expected 6 but found 4  (indentation)
  53:7      error    wrong indentation: expected 8 but found 6  (indentation)
  63:81     error    line too long (233 > 80 characters)  (line-length)
  65:81     error    line too long (83 > 80 characters)  (line-length)
  68:81     error    line too long (142 > 80 characters)  (line-length)
  70:81     error    line too long (83 > 80 characters)  (line-length)
  85:7      error    wrong indentation: expected 8 but found 6  (indentation)
  95:81     error    line too long (154 > 80 characters)  (line-length)
  97:81     error    line too long (83 > 80 characters)  (line-length)
  100:81    error    line too long (144 > 80 characters)  (line-length)
  102:81    error    line too long (83 > 80 characters)  (line-length)
  117:7     error    wrong indentation: expected 8 but found 6  (indentation)
  119:7     error    wrong indentation: expected 8 but found 6  (indentation)
  129:81    error    line too long (158 > 80 characters)  (line-length)
  131:81    error    line too long (83 > 80 characters)  (line-length)
  134:81    error    line too long (148 > 80 characters)  (line-length)
  136:81    error    line too long (83 > 80 characters)  (line-length)
  151:7     error    wrong indentation: expected 8 but found 6  (indentation)
  153:7     error    wrong indentation: expected 8 but found 6  (indentation)
  161:81    error    line too long (151 > 80 characters)  (line-length)
  163:81    error    line too long (83 > 80 characters)  (line-length)
  166:81    error    line too long (141 > 80 characters)  (line-length)
  168:81    error    line too long (83 > 80 characters)  (line-length)
  183:7     error    wrong indentation: expected 8 but found 6  (indentation)
  185:7     error    wrong indentation: expected 8 but found 6  (indentation)
  194:81    error    line too long (155 > 80 characters)  (line-length)
  196:81    error    line too long (83 > 80 characters)  (line-length)
  199:81    error    line too long (145 > 80 characters)  (line-length)
  201:81    error    line too long (83 > 80 characters)  (line-length)

This happens with any generated advisor-result.yml file, and seems to break tools like yq.

@sschuberth
Copy link
Member

The line too long "errors" are completely optional; line length may only matter for human readability, but not for machine progressing.

And the wrong indentation "errors" are reported for list entries like

projects:
- id: "Maven:com.vdurmont:semver4j:3.1.0"

where yamllint wants

projects:
  - id: "Maven:com.vdurmont:semver4j:3.1.0"

instead. But actually, both are valid according to the YAML specs. We simply rely on Jackson's default here, which is to omit unneeded indentation to reduce file size.

and seems to break tools like yq.

Actually, I know several parties are successfully parsing ORT results files with yq and have not reported any issues so far. If there was an issue, I'm rather tempted to say that yq is overly strict and does not support all syntax that is valid according to the YAML specs.

@dgutson
Copy link
Author

dgutson commented Jun 29, 2023

@sschuberth there are two reasons of this issue:

  • one is that we have yamllint in our CI pipelines, which configuration now we'll have to modify
  • the most important one, I'll get an intern and thought, as I mentioned to @tsteenbe , that this could be a good harmless first issue to get acquainted with the code. Shall you have another proposal, pls let me know. I don't have your email address but Thomas has mine.

@sschuberth
Copy link
Member

sschuberth commented Jun 29, 2023

we have yamllint in our CI pipelines, which configuration now we'll have to modify

IMO it's really over the top that yamllint reports these as "errors". These should be "hints" at most.

On the other hand, yamllint itself says that is checks for "cosmetic problems such as lines length, trailing spaces, indentation, etc." (emphasis mine). I'm not sure how valuable it is to check YAML files that are (mostly) meant for machine processing (in this case) for cosmetic problems...

that this could be a good harmless first issue to get acquainted with the code.

You mean the issue of addressing "errors" reported by yamllint? I'd disagree that that's a good issue to address to get started: First of all, nothing is really broken, and secondly, we have some tests in the code base that strictly check actual results to match expected results incl. exact formatting, and these would break without an update of the expected results.

Shall you have another proposal, pls let me know.

That largely depends on the skills of the intern...

I don't have your email address but Thomas has mine.

You can retrieve my email address easily from the Git history of this project.

@dgutson
Copy link
Author

dgutson commented Jun 29, 2023

Ok. I'll file an issue in yamllint proposing to lower the severity of those 2 findings, will do more tests with yq to confirm it is a bug, in which case I'll create an issue there too.

@dgutson dgutson closed this as not planned Won't fix, can't repro, duplicate, stale Jun 29, 2023
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

No branches or pull requests

2 participants