Add `check-keys` option to `quoted-strings` rule #618

HenryGessau · 2023-11-23T01:40:38Z

Closes #617

coveralls · 2023-11-23T01:41:25Z

coverage: 99.414% (+0.09%) from 99.322%
when pulling 9ef0479 on HenryGessau:key-quoted-strings
into e5fdfd2 on adrienverge:master.

adrienverge

Hello Henry, and thanks for this pull request.

I agree with what you wrote in #617 👍
However, I don't see the need for a new rule key-quoted-strings just for keys, moreover in such a scenario the rule quoted-strings should then be renamed value-quoted-strings. In my opinion the required quote type will always be the same (nobody wants ' for keys and " for values), so what about using quoted-strings for all quoted strings (whatever their position)? It would also avoid a lot of code duplication.

Please tell me what you think before I look at the code in detail.

PS: You can squash all commits into one for easier review and merging.

adrienverge · 2023-11-27T16:57:56Z

docs/conf.py

@@ -13,6 +13,7 @@

 extensions = [
    'sphinx.ext.autodoc',
+    'sphinx.ext.autosectionlabel',


In the updated doc strings, I added references to docs for other rules.
For example:

... see the :ref:`quoted-strings` rule ...

This required that section labels be generated by the tooling.

However, since I am going to remove the key-quoted-strings rule, I will no longer need to reference other rule docs, so this change will not be needed.

HenryGessau · 2023-11-27T19:10:23Z

I don't see the need for a new rule key-quoted-strings just for keys, moreover in such a scenario the rule quoted-strings should then be renamed value-quoted-strings. In my opinion the required quote type will always be the same (nobody wants ' for keys and " for values), so what about using quoted-strings for all quoted strings (whatever their position)? It would also avoid a lot of code duplication.

I came across a project where they want quotes to be required if there is a $ in value strings, but not quoted for $ in key strings. That was the only reason why I proposed support for separate quoting rules for keys, but now that I think about it, that's a rather esoteric style. (If there is real demand, such styles could be perhaps supported later by adding keys-extra-required and values-extra-required options to the existing rule.)

I'll pare this PR down to remove the key-quoted-strings rule. And, yes, I will squash the commits.

HenryGessau · 2023-11-30T15:11:04Z

@adrienverge I have updated this for your review.

adrienverge

Hello Henry, this looks good!

Please see my few requests.

Note: I haven't looked carefully at tests cases yet (see my comment), but at first glance they seem to cover many cases.

adrienverge · 2023-12-10T16:35:54Z

yamllint/rules/common.py

+def is_key(token):
+    return token and isinstance(token, yaml.KeyToken)


Since this function is only used at one place, I propose to inline it there. And I believe it's not needed to check token and because None can be passed to isinstance().

(see my suggestion in the comment below ↓)

adrienverge · 2023-12-10T16:37:08Z

yamllint/rules/quoted_strings.py

+#. With ``quoted-strings: {required: only-when-needed, check-keys: true,
+   extra-required: ["[:]"]}``
+
+   the following code snippet would **FAIL**:
+   ::
+
+    foo:bar: baz
+
+   the following code snippet would **PASS**:
+   ::
+
+    "foo:bar": baz
+


Neat example 👍

(Can you remove the empty line at the end?)

Sure, but why was there an empty line at the end before I added this example?

I guess it was introduced by commit 352e1a9, anyway, thanks for fixing it 👍

yamllint/rules/quoted_strings.py

adrienverge · 2023-12-10T17:26:01Z

tests/rules/test_quoted_strings.py

+    def conf(self, options):
+        conf_with_options = (f"{self.rule_id}:\n"
+                             "  check-keys: true\n")
+        for option in options:
+            conf_with_options += f"  {option}\n"
+        return conf_with_options


If I understand correctly this function allows writing:

conf = self.conf(['required: only-when-needed', 'extra-allowed: [^ftp://]', 'extra-required: [^http://]'])

instead:

conf = ('quoted-strings: {check-keys: true\n' ' required: only-when-needed\n' ' extra-allowed: [^ftp://]\n' ' extra-required: [^http://]\n')

If you don't mind I prefer the classic version, because:
- there isn't a big added value in number of lines
- in my opinion it's a bit worse for readability
- the rest of yamllint code uses the classic version.

What do you think?

adrienverge · 2023-12-10T17:31:38Z

tests/rules/test_quoted_strings.py

+    key_strings = ('---\n'
+                   'true: 2\n'
+                   '123: 3\n'
+                   'foo1: 4\n'
+                   '"foo2": 5\n'
+                   '"false": 6\n'
+                   '"234": 7\n'
+                   '\'bar\': 8\n'
+                   '!!str generic_string: 9\n'
+                   '!!str 456: 10\n'
+                   '!!str "quoted_generic_string": 11\n'
+                   '!!binary binstring: 12\n'
+                   '!!int int_string: 13\n'
+                   '!!bool bool_string: 14\n'
+                   '!!bool "quoted_bool_string": 15\n'
+                   # Sequences and mappings
+                   '? - 16\n'
+                   '  - 17\n'
+                   ': 18\n'
+                   '[119, 219]: 19\n'
+                   '? a: 20\n'
+                   '  "b": 21\n'
+                   ': 22\n'
+                   '{a: 123, "b": 223}: 23\n'
+                   # Multiline strings
+                   '? |\n'
+                   '  line 1\n'
+                   '  line 2\n'
+                   ': 27\n'
+                   '? >\n'
+                   '  line 1\n'
+                   '  line 2\n'
+                   ': 31\n'
+                   '?\n'
+                   '  line 1\n'
+                   '  line 2\n'
+                   ': 35\n'
+                   '?\n'
+                   '  "line 1\\\n'
+                   '   line 2"\n'
+                   ': 39\n')


I understand you propose to reuse a variable for testing various rule configurations. It is smart, but doing so has already been discussed in other pull requests and was finally rejected, because it makes it hard to read the tests and understand where it should fail / make sure there is no human error. (For instance, it's hard for me to review and visually understand/check test cases.)

I know it's a lot of code duplication, but inside tests we allow it if it helps for readability and maintenance.

No problem. Readability and maintenance are important.

HenryGessau · 2023-12-10T19:21:52Z

@adrienverge I don't see how the CI errors are related to my change?

adrienverge

Please ignore the failing tests, they were due to a small bug in pathspec 0.12.0, see cpburnz/python-pathspec#84.

Thanks for your contribution!

adrienverge reviewed Nov 27, 2023

View reviewed changes

HenryGessau force-pushed the key-quoted-strings branch from 2d6ca98 to 16cc4d7 Compare November 27, 2023 20:52

HenryGessau changed the title ~~String quoting rules for keys~~ Add check-keys option to quoted-strings rule Nov 27, 2023

adrienverge reviewed Dec 10, 2023

View reviewed changes

Add check-keys option to quoted-strings rule

9ef0479

HenryGessau force-pushed the key-quoted-strings branch from fa17b34 to 9ef0479 Compare December 10, 2023 19:19

adrienverge approved these changes Dec 11, 2023

View reviewed changes

adrienverge merged commit 3288d05 into adrienverge:master Dec 11, 2023
4 of 7 checks passed

HenryGessau deleted the key-quoted-strings branch December 11, 2023 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `check-keys` option to `quoted-strings` rule #618

Add `check-keys` option to `quoted-strings` rule #618

HenryGessau commented Nov 23, 2023

coveralls commented Nov 23, 2023 •

edited

Loading

adrienverge left a comment

adrienverge Nov 27, 2023

HenryGessau Nov 27, 2023

HenryGessau commented Nov 27, 2023

HenryGessau commented Nov 30, 2023

adrienverge left a comment

adrienverge Dec 10, 2023

HenryGessau Dec 10, 2023

adrienverge Dec 10, 2023

HenryGessau Dec 10, 2023

adrienverge Dec 11, 2023

adrienverge Dec 10, 2023

HenryGessau Dec 10, 2023

adrienverge Dec 10, 2023

HenryGessau Dec 10, 2023

HenryGessau commented Dec 10, 2023

adrienverge left a comment

		def is_key(token):
		return token and isinstance(token, yaml.KeyToken)

Add check-keys option to quoted-strings rule #618

Add check-keys option to quoted-strings rule #618

Conversation

HenryGessau commented Nov 23, 2023

coveralls commented Nov 23, 2023 • edited Loading

adrienverge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HenryGessau commented Nov 27, 2023

HenryGessau commented Nov 30, 2023

adrienverge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HenryGessau commented Dec 10, 2023

adrienverge left a comment

Choose a reason for hiding this comment

Add `check-keys` option to `quoted-strings` rule #618

Add `check-keys` option to `quoted-strings` rule #618

coveralls commented Nov 23, 2023 •

edited

Loading