plain-scalar: always rule misses optional quotes

[wolfgangwalther]

I noticed a case where plain-scalar: always would not report an error, even though the quotes in use were optional.

Then I ran eslint --fix with plain-scalar: never temporarily on my whole code-base, which added quotes everywhere. After running eslint --fix with plain-scalar: always again, I still had a diff remaining. However, I would expect this action to be 100% reversible. The remaining diff means that there are some optional quotes, that are not caught by plain-scalar: always.

The following diff to the unit tests, highlights those cases:

diff --git a/tests/fixtures/rules/plain-scalar/valid/test-input.yml b/tests/fixtures/rules/plain-scalar/valid/test-input.yml
index c50829b..1c5731f 100644
--- a/tests/fixtures/rules/plain-scalar/valid/test-input.yml
+++ b/tests/fixtures/rules/plain-scalar/valid/test-input.yml
@@ -1,2 +1,17 @@
 # {}
 a: b
+
+c,c: d,d
+"e,e": "f,f"
+
+g:g: h:h
+"i:i": "j:j"
+
+k|k: l|l
+"m|m": "n|n"
+
+o{o: p{p
+"q{q": "r{r"
+
+s}s: t}t
+"u}u": "v}v"

Adding those test-cases to the valid fixtures makes the tests still pass. However, in each of those cases the same input is given once with quotes and once without. The plain-scalar rule should throw errors in all of the quoted examples here, because those quotes are optional.

[wolfgangwalther]

I think the following check needs to take into account the position of the character in the string and/or whether it's followed by whitespace or not (in the case of :):

eslint-plugin-yml/src/rules/plain-scalar.ts

Lines 110 to 112 in 576f58f

	if (SYMBOLS.has(char)) {
	return false
	}

[ota-meshi]

Thank you for this issue.
I know there are false positives in this rule, but we should carefully check how to safely remove the quotes.

e.g.

- ["e,e": "f,f"]
- [e,e: f,f]

Also, removing quotes from them can be difficult for humans to read, so I think it may need to be an option.

[wolfgangwalther]

That makes sense. For me, the most important factor is consistency. I want the plugin to force a decision. Even if the quotes are not needed from the spec in this case, I would be fine, if they were added consistently. So as long as changing the setting to never and back to always will always revert 100% of the changes, that would be cool.

Implementation wise this could take a real "dumb" approach: Could we do the following?

When a quoted value is found:

• parse the given yaml to json (either the whole document, or the smallest possible "scope" - just the value in quotes is probably not enough, as it depends on context as your example shows)
• remove the quotes
• parse the same scope as json again
• compare the two jsons - if they match, it was safe to remove the quotes

This would avoid the need to try to replicate the spec.

WDYT?

[wolfgangwalther]

Also, removing quotes from them can be difficult for humans to read, so I think it may need to be an option.

The suggestion I made above, does not take this into account. I see that point, but I fear the "readability" part might be subjective and a lot harder to implement. One general approach to this would be to say: Whenever a scalar can not be represented as a plain scalar in any one context, add quotes to this scalar in all contexts.

Although I don't really know where we would end up with that. How many different contexts would we need to take into consideration? Would we add "too many" quotes because of that?

[ota-meshi]

When a quoted value is found:

• parse the given yaml to json (either the whole document, or the smallest possible "scope" - just the value in quotes is probably not enough, as it depends on context as your example shows)
• remove the quotes
• parse the same scope as json again
• compare the two jsons - if they match, it was safe to remove the quotes

This would avoid the need to try to replicate the spec.

That idea sounds good to me. I don't want to re-parse the entire document as much as possible, but it can be difficult for me.

The suggestion I made above, does not take this into account. I see that point, but I fear the "readability" part might be subjective and a lot harder to implement.

I was thinking of using pattern to exclude it from the check. But I forgot that the option already existed 😅.
https://ota-meshi.github.io/eslint-plugin-yml/rules/plain-scalar.html#options