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

Add a "quote-percent" option to the "quoted-strings" rule #244

Closed
wants to merge 1 commit into from
Closed

Add a "quote-percent" option to the "quoted-strings" rule #244

wants to merge 1 commit into from

Conversation

leofeyer
Copy link

I really like the new required: only-when-needed option that has been added to the quoted-strings rule in version 1.21.0. Thank you for that.

When linting Symfony configuration files, there is a kind of false-positive though: Symfony requires placeholders such as %kernel.project_dir% or %env(DATABASE_URL)% to be quoted. Now even though this deviates from the YAML standard, it would be nice if yamllint would support this somehow.

This PR outlines a possible solution by adding a quote-percent option to the quoted-strings rule. Tests are still missing, because I wanted to discuss the implementation first. WDYT?

@leofeyer leofeyer changed the title Add the "quote-percent" option to the "quoted-strings" rule Add a "quote-percent" option to the "quoted-strings" rule Mar 31, 2020
@coveralls
Copy link

coveralls commented Mar 31, 2020

Coverage Status

Coverage decreased (-0.09%) to 97.786% when pulling b41de1d on leofeyer:patch-1 into 542ae75 on adrienverge:master.

@adrienverge
Copy link
Owner

adrienverge commented Mar 31, 2020

Hello @leofeyer, thanks for your proposal.

I'm against features specific to one program (including Symfony), and I think quote-percent usage is too narrow to be a good addition to yamllint.

However, we could imagine something more general, like a new option needed-extra-regex (or any better wording)?

Then for Symfony you could use:

quoted-strings:
  required: only-when-needed
  needed-extra-regex: ^%.*%$

If you like the idea, feel free to open a new PR!

@leofeyer
Copy link
Author

What about making START_TOKENS configurable?

CONF = {'quote-type': ('any', 'single', 'double'),
        'required': (True, False, 'only-when-needed'),
        'start-tokens': set()}
DEFAULT = {'quote-type': 'any',
           'required': True,
           'start-tokens': {'#', '*', '!', '?', '@', '`', '&', ',', '-', '{', '}', '[', ']', ':'}}

I am not familiar with Python, so no idea if that is the correct syntax.

@adrienverge
Copy link
Owner

It wouldn't be a general solution. What if I want to quote strings that look like phone numbers, for example? A regex would be more configurable.

@leofeyer
Copy link
Author

I agree. 👍 As I said, I am not familiar with Python, but I will try my best to create a PR that implements the needed-extra-regex option.

@adrienverge
Copy link
Owner

Cool!

I recommend starting by writing tests in tests/rules/test_quoted_strings.py. Write exactly what you expect from the rule (including corner cases), then it will be easy to check your feature while you write it (using python3 -m unittest tests.rules.test_quoted_strings).

@leofeyer
Copy link
Author

leofeyer commented Apr 1, 2020

See #246. 🎉

@leofeyer leofeyer closed this Apr 1, 2020
@leofeyer leofeyer deleted the patch-1 branch April 1, 2020 07:48
adrienverge added a commit that referenced this pull request Apr 13, 2020
Change implementation of `required: only-when-needed`, because
maintaining a list of `START_TOKENS` and just looking at the first
character of string values has proven to be partially broken.

Cf. discussion at
#246 (comment).

Fixes #242 and
#244.
adrienverge added a commit that referenced this pull request Apr 13, 2020
Change implementation of `required: only-when-needed`, because
maintaining a list of `START_TOKENS` and just looking at the first
character of string values has proven to be partially broken.

Cf. discussion at
#246 (comment).

Fixes #242 and
#244.
adrienverge added a commit that referenced this pull request Apr 15, 2020
Change implementation of `required: only-when-needed`, because
maintaining a list of `START_TOKENS` and just looking at the first
character of string values has proven to be partially broken.

Cf. discussion at
#246 (comment).

Fixes #242 and
#244.
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

Successfully merging this pull request may close these issues.

3 participants