feat: double_quotes

[simojoe]

This pull request adds a new rule to wdl.

• Rule Name: double_quotes
• Rule Kind: Lint warning
• Rule Code: v1::W012
• Packages: wdl-grammar

Warns about strings defined with single-quotes. This ignores anything located inside the command section. Tests are not exhaustive, there are quite a lot of things that are using strings. Should they still be added?

Before submitting this PR, please make sure:

• You have added a few sentences describing the PR here.
• You have added yourself or the appropriate individual as the assignee.
• You have added at least one relevant code reviewer to the PR.
• Your code builds clean without any errors or warnings.
• You have added an entry to the relevant CHANGELOG.md (see
  "keep a changelog" for more information).
• Your commit messages follow the conventional commit style.
• Your changes are squashed into a single commit (unless there is a really
  good, articulated reason as to why there should be more than one).

Rule specific checks:

• You have added the rule as an entry within the the package-specific rule
  tables (wdl-ast/src/v1.rs for AST-based rules and
  wdl-grammar/src/v1.rs for parse tree-based rules).
• You have added the rule as an entry within the the global rule
  table at RULES.md.
• You have added the rule to the appropriate fn rules().
  • Validation rules added to wdl-ast should be added to fn rules() within
    wdl-ast/src/v1/validation.rs.
  • Lint rules added to wdl-ast should be added to fn rules() within wdl-ast/src/v1/lint.rs.
  • Validation rules added to wdl-grammar should be added to fn rules() within
    wdl-grammar/src/v1/validation.rs.
  • Lint rules added to wdl-grammar should be added to fn rules() within
    wdl-grammar/src/v1/lint.rs.
• You have added a test that covers every possible setting for the rule
  within the file where the rule is implemented.
• You have run wdl-gauntlet --save-config to ensure that all of the rules
  added/removed are now reflected in the baseline configuration file
  (Gauntlet.toml).

[a-frantz]

Does this work for nested placeholder strings? I haven't checked out the branch to test myself. e.g. String nested = "Hello ~{if alien then 'world' else planet}!". The inner 'world' string should be flagged. There are other cases we probably want to check as well.

I'm fine with skipping over the command block for now. That seems like a bonus check to me, that shouldn't hold up the other test conditions.

[adthrasher]

Does this work for nested placeholder strings? I haven't checked out the branch to test myself. e.g. String nested = "Hello ~{if alien then 'world' else planet}!". The inner 'world' string should be flagged. There are other cases we probably want to check as well.

I'm fine with skipping over the command block for now. That seems like a bonus check to me, that shouldn't hold up the other test conditions.

It does appear to catch that condition.

[adthrasher]

I rebased this to get the latest changes. Looking at the Arena results, it does appear to be checking within the command block.

[a-frantz]

I rebased this to get the latest changes. Looking at the Arena results, it does appear to be checking within the command block.

Ok so that's a good thing I think. As long as it doesn't apply to literal Bash code. e.g. look at this hypothetical command block

command <<<
    echo 'this Bash string should be ignored'
    echo "~{if foo then 'this should be flagged' else 'this one too'}"
>>>

[adthrasher]

I rebased this to get the latest changes. Looking at the Arena results, it does appear to be checking within the command block.

Ok so that's a good thing I think. As long as it doesn't apply to literal Bash code. e.g. look at this hypothetical command block

command <<<
    echo 'this Bash string should be ignored'
    echo "~{if foo then 'this should be flagged' else 'this one too'}"
>>>

To clarify, it filters to v1::Rule::string instances. So it will ignore anything that isn't part of a WDL instruction in the command block. So in your example, it ignores the first line, but will catch the two embedded strings.

[a-frantz]

I rebased this to get the latest changes. Looking at the Arena results, it does appear to be checking within the command block.

Ok so that's a good thing I think. As long as it doesn't apply to literal Bash code. e.g. look at this hypothetical command block

command <<<
    echo 'this Bash string should be ignored'
    echo "~{if foo then 'this should be flagged' else 'this one too'}"
>>>

To clarify, it filters to v1::Rule::string instances. So it will ignore anything that isn't part of a WDL instruction in the command block. So in your example, it ignores the first line, but will catch the two embedded strings.

That's perfect then. Let's add a my example as an actual test case for sanity, update Arena, and then this should be good to merge.

[adthrasher]

I rebased this to get the latest changes. Looking at the Arena results, it does appear to be checking within the command block.

Ok so that's a good thing I think. As long as it doesn't apply to literal Bash code. e.g. look at this hypothetical command block

command <<<
    echo 'this Bash string should be ignored'
    echo "~{if foo then 'this should be flagged' else 'this one too'}"
>>>

To clarify, it filters to v1::Rule::string instances. So it will ignore anything that isn't part of a WDL instruction in the command block. So in your example, it ignores the first line, but will catch the two embedded strings.

That's perfect then. Let's add a my example as an actual test case for sanity, update Arena, and then this should be good to merge.

Test added. I've updated Arena, but I was going to hold off until stjudecloud/workflows#157 is merged. However, I can commit the Arena warnings and move ahead with this first if you prefer.