new check: variables that should be quoted#379
Closed
laumann wants to merge 1 commit intopkgcore:masterfrom
laumann:unquoted-variables
Closed
new check: variables that should be quoted#379laumann wants to merge 1 commit intopkgcore:masterfrom laumann:unquoted-variables
laumann wants to merge 1 commit intopkgcore:masterfrom
laumann:unquoted-variables
Conversation
Codecov Report
@@ Coverage Diff @@
## master #379 +/- ##
==========================================
- Coverage 95.65% 95.61% -0.04%
==========================================
Files 55 55
Lines 7573 7551 -22
Branches 1844 1842 -2
==========================================
- Hits 7244 7220 -24
- Misses 204 205 +1
- Partials 125 126 +1
Continue to review full report at Codecov.
|
laumann
commented
May 18, 2022
Member
|
The implementation looks very good! Nice usage of tree-sitter-bash! |
arthurzam
requested changes
May 18, 2022
arthurzam
reviewed
May 18, 2022
mgorny
reviewed
May 19, 2022
thesamesam
reviewed
May 19, 2022
Contributor
Author
|
Also @floppym if you're interested |
Contributor
Author
|
For the record, here's the list of warnings currently produced by this check Total: 457 |
Contributor
Author
|
One thing that concerns me is the warnings produced for net-misc/openssh: These all appear to be false positives - but I haven't yet figured out how to address these. EDIT: I have a strong suspicion that the openssh ebuilds aren't parsed properly by tree-sitter-bash. |
Contributor
Yeah, it looks like it gets confused by this line: Removing that line from the ebuild makes the false positives go away. |
arthurzam
reviewed
May 20, 2022
Member
Some more investigating, and the following 2 lines [1] are very confusing tree-sitter (21 children for root instead of 81). If I break those 2 lines into 4 lines of assignment, everything works correctly. EDIT: fixed in gentoo/gentoo@de045c1 |
Contributor
Author
|
Thanks @arthurzam @floppym for helping me hunt down the false positives. It's pretty clear the root cause for net-misc/openssh is a bug in the tree-sitter-bash parser, I've opened an issue detailing where the parser goes wrong: tree-sitter/tree-sitter-bash#122 |
Contributor
Author
|
but it gave me an idea: if the parser has an ERROR node in the tree, maybe we should just skip this check to avoid potential false positives? |
Contributor
Author
|
Just an update: now ebuilds that tree-sitter-bash doesn't parse correctly are skipped for this check to avoid false positives. That results in the current report: Total: 383 |
Contributor
Author
|
Updated to check eclasses as well: |
radhermit
reviewed
May 24, 2022
Contributor
Author
|
OK, so a bit of an overhaul based on @radhermit's input
|
laumann
commented
May 26, 2022
laumann
commented
May 26, 2022
arthurzam
requested changes
May 27, 2022
Contributor
Author
|
With the latest rewrite you can do: and get results for both ebuilds and eclasses: Total: 209 (grep UnquotedVariable |wc -l) |
arthurzam
requested changes
May 27, 2022
Member
arthurzam
left a comment
arthurzam
left a comment
There was a problem hiding this comment.
In my opinion, this is on the finish line for being accepted and merged. Very small nit-picks before we can merge it.
This is based on the repoman check EbuildQuote that reports instances of some variables that should be quoted in certain contexts. See: https://gitweb.gentoo.org/proj/portage.git/tree/repoman/lib/repoman/modules/linechecks/quotes/quotes.py?h=portage-3.0.30 Closes: #363 Signed-off-by: Thomas Bracht Laumann Jespersen <t@laumann.xyz>
arthurzam
approved these changes
May 27, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is based on the repoman check EbuildQuote that reports instances of
some variables that should be quoted in certain contexts.
See: https://gitweb.gentoo.org/proj/portage.git/tree/repoman/lib/repoman/modules/linechecks/quotes/quotes.py?h=portage-3.0.30
Closes: #363
Signed-off-by: Thomas Bracht Laumann Jespersen t@laumann.xyz