Skip to content

Conversation

@rgeraskin
Copy link
Contributor

@rgeraskin rgeraskin commented Aug 5, 2022

This PR implements skip-quoted-quote allow-quoted-quotes option that allows strings like:

  • 'foo"bar' on quote-type: double
  • "foo'bar" on quote-type: single

Closes use-case with quotes inside the string from #424. But still no solution for escape sequences.

allows strings like 'foo"bar' on `quote-type: double` and vice versa
@coveralls
Copy link

coveralls commented Aug 6, 2022

Coverage Status

Coverage increased (+0.001%) to 99.171% when pulling dd9ec72 on rgeraskin:master into e319a17 on adrienverge:master.

Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @rgeraskin, thanks for contributing!

Can you add tests? Make sure the new tests take example from existing ones and insert nicely, for a full coverage and easier code maintenance.

About the new option name: I propose allow-quoted-quotes instead of skip-quoted-quote, to use conventional "allow"/"forbid" of yamllint options, and the plural form.

Closes use-case with quotes inside the string from #424. But still no solution for escape sequences.

What do you mean by "no solution for escape sequences"?

@rgeraskin
Copy link
Contributor Author

Hello @adrienverge, thanks for the detailed and fast feedback!

I've added tests to cover all cases for this feature. Also, I agree with you about the feature name, I've renamed it to allow-quoted-quotes.

About "no solution for escape sequences". As stated in #424:

Another case is when quote-type is set to double, but the string contains escape sequences. In those cases, single quote is better, since the content is not interpreted (so we don't need to double backslashes):

So to achieve this functionality yamllint should be able to determine escape sequences in a string and not to complain about wrong quotes even through quote-type setting. It's not so easy as the feature from this PR.

@adrienverge
Copy link
Owner

Thanks for these modifications. The code in yamllint/ looks perfect.

However for tests I would really prefer explicit YAML blocks, even if it causes a lot of repetition. It allows easy and reliable visual check of what we expect for correct/wrong YAML code.

Example:

    def test_allow_quoted_quotes(self):
        conf = 'quoted-strings: {required: true, quote-type: '"', allow-quoted-quotes: true}\n'
        self.check('---\n'
                   'foo1: "[barbaz]"\n'
                   'foo2: "[bar'baz]"\n'
                   conf,
                   problem=...)

        conf = 'quoted-strings: {required: false, quote-type: '"', allow-quoted-quotes: true}\n'
        self.check('---\n'
                   'foo1: "[barbaz]"\n'
                   'foo2: "[bar'baz]"\n'
                   conf,
                   problem=...)

        # quote-type: any should accept both:
                   'foo3: '[bar"baz]'\n'
                   'foo3: "[bar'baz]"\n'
	# etc.

Use explicit YAML blocks instead of loops. It allows easy
and reliable visual check of what we expect for correct/wrong YAML code.
@rgeraskin
Copy link
Contributor Author

@adrienverge I've updated tests for allow-quoted-quotes following your advice. Now they looks similar to other tests.

@rgeraskin rgeraskin changed the title quoted-strings: add skip-quoted-quote option quoted-strings: add allow-quoted-quotes option Aug 7, 2022
Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Roman, thanks for this cool addition to yamllint, and for your reactivity.

Let's merge this!

@adrienverge adrienverge merged commit 352e1a9 into adrienverge:master Aug 7, 2022
@adrienverge
Copy link
Owner

I see that the generated documentation is wrong, because the first backslash is removed:
Capture d’écran du 2022-08-07 13-51-52

Do you think you could fix this in a new pull request? (Docs can be generated locally using python setup.py build_sphinx.)

@rgeraskin
Copy link
Contributor Author

@adrienverge my bad, I've fixed it in #490

thank you for review!

@bendem
Copy link

bendem commented Sep 12, 2022

Thank you for this, this is great!

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.

4 participants