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

hyphen: Add min-spaces-after and check-scalars options #606

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 56 additions & 1 deletion tests/rules/test_hyphens.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,25 @@

from tests.common import RuleTestCase

from yamllint import config


class HyphenTestCase(RuleTestCase):
rule_id = 'hyphens'

def test_disabled(self):
conf = 'hyphens: disable'
self.run_disabled_test('hyphens: disable')
self.run_disabled_test('hyphens:\n'
' max-spaces-after: 5\n'
' min-spaces-after: -1\n')
self.run_disabled_test('hyphens:\n'
' max-spaces-after: -1\n'
' min-spaces-after: -1\n')
self.run_disabled_test('hyphens:\n'
' max-spaces-after: -1\n'
' min-spaces-after: 0\n')

def run_disabled_test(self, conf):
self.check('---\n'
'- elem1\n'
'- elem2\n', conf)
Expand Down Expand Up @@ -51,6 +64,9 @@ def test_disabled(self):
' subobject:\n'
' - elem1\n'
' - elem2\n', conf)
self.check('---\n'
'object:\n'
' -elem2\n', conf)

def test_enabled(self):
conf = 'hyphens: {max-spaces-after: 1}'
Expand Down Expand Up @@ -103,3 +119,42 @@ def test_max_3(self):
' b:\n'
' - elem1\n'
' - elem2\n', conf, problem1=(4, 9), problem2=(5, 9))

def test_invalid_spaces(self):
conf = 'hyphens: {max-spaces-after: 0}'
self.assertRaises(config.YamlLintConfigError, self.check, '', conf)

conf = 'hyphens: {min-spaces-after: 3}'
self.assertRaises(config.YamlLintConfigError, self.check, '', conf)

def test_min_space(self):
conf = 'hyphens: {max-spaces-after: 4, min-spaces-after: 3}'
self.check('---\n'
'object:\n'
' - elem1\n'
' - elem2\n', conf)
self.check('---\n'
'object:\n'
' - elem1\n'
' - elem2: -foo\n'
'-bar:\n', conf)
self.check('---\n'
'object:\n'
' - elem1\n'
' - elem2\n', conf, problem1=(3, 6), problem2=(4, 6))

conf = ('hyphens:\n'
' max-spaces-after: 4\n'
' min-spaces-after: 3\n'
' check-scalars: true\n')
self.check('---\n'
'foo\n'
'-bar\n', conf)
self.check('---\n'
'object:\n'
' - elem1\n'
' - elem2\n'
'key: -value\n', conf, problem=(5, 6))
self.check('---\n'
'list:\n'
' -value\n', conf, problem=(3, 3))
104 changes: 96 additions & 8 deletions yamllint/rules/hyphens.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,20 @@
.. rubric:: Options

* ``max-spaces-after`` defines the maximal number of spaces allowed after
hyphens.
hyphens. Set to a negative integer if you want to allow any number of
spaces.
Comment on lines 21 to +23
Copy link
Owner

Choose a reason for hiding this comment

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

  1. Suggested change
    * ``max-spaces-after`` defines the maximal number of spaces allowed after
    hyphens.
    hyphens. Set to a negative integer if you want to allow any number of
    spaces.
    * ``max-spaces-after`` defines the maximal number of spaces allowed after
    hyphens. Set to ``-1`` if you want to allow any number of spaces.
    (just in case we want to reserve other magic values like -2 in the future)
  2. Can you place max-spaces-after after min-spaces-after?

* ``min-spaces-after`` defines the minimal number of spaces expected after
hyphens. Set to a negative integer if you want to allow any number of
spaces. When set to a positive value, cannot be greater than
``max-spaces-after``.
Comment on lines +24 to +27
Copy link
Owner

Choose a reason for hiding this comment

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

I propose to be a bit simpler:

Suggested change
* ``min-spaces-after`` defines the minimal number of spaces expected after
hyphens. Set to a negative integer if you want to allow any number of
spaces. When set to a positive value, cannot be greater than
``max-spaces-after``.
* ``min-spaces-after`` defines the minimal number of spaces expected after
hyphens. The default and minimum value is ``1``.

(the rest is obvious and will throw a configuration error if needed)

* YAMLLint will consider ``-xx`` as a scalar. However you may consider
that, in your context, such a syntax is a typo and is actually a sequence
and as a consequence there should be a space after the hyphen. As this is
not a standard behaviour, you explicitly need to enable this control by
setting the option ``check-scalars`` to ``true``. **Use with caution**
as all scalars will be checked and non-solvable false positive might be
identified. Has no effect when set to ``true`` but ``min-spaces-after``
is disabled (< 0).

.. rubric:: Default values (when enabled)

Expand All @@ -28,6 +41,8 @@
rules:
hyphens:
max-spaces-after: 1
min-spaces-after: -1 # Disabled
Copy link
Owner

Choose a reason for hiding this comment

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

# Disabled isn't noted for other rules with -1 as default value (braces, brackets...) For consistency, I propose not to add here neither.

Or even better: let's set min-spaces-after: 1 as the default (instead of -1), and as the disabled value, since having less than one space results in a syntax error anyway. It should add some clarity for users. What do you think?

Also can you place min-spaces-after option before max-spaces-after?

check-scalars: false

.. rubric:: Examples

Expand Down Expand Up @@ -72,24 +87,97 @@
- key
- key2
- key42

#. With ``hyphens: {min-spaces-after: 3}``

the following code snippet would **PASS**:
::

list:
- key
- key2
- key42
-foo: # starter of a new sequence named "-foo";
# without the colon, a syntax error will be raised.
Comment on lines +100 to +101
Copy link
Owner

Choose a reason for hiding this comment

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

This is a bit irrelevant to the example, and only adds confusion in my opinion. Let's remove it?


the following code snippet would **FAIL**:
::

- key
- key2
- key42

#. With ``hyphens: {min-spaces-after: 3, check-scalars: true}``

the following code snippet would **PASS**:
::

list:
- key
- key2
- key42
key: -value

the following code snippets would **FAIL**:
::

---
-item0

::

sequence:
-key # Mind the spaces before the hyphen to enforce
# the sequence and avoid a syntax error
Comment on lines +127 to +131
Copy link
Owner

Choose a reason for hiding this comment

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

Same here: this seems irrelevant and adds confusion: let's remove it.

"""


import yaml

from yamllint.linter import LintProblem
from yamllint.rules.common import spaces_after


ID = 'hyphens'
TYPE = 'token'
CONF = {'max-spaces-after': int}
DEFAULT = {'max-spaces-after': 1}
CONF = {'max-spaces-after': int,
'min-spaces-after': int,
'check-scalars': bool}
DEFAULT = {'max-spaces-after': 1,
'min-spaces-after': -1,
'check-scalars': False}


def VALIDATE(conf):
if conf['max-spaces-after'] == 0:
return '"max-spaces-after" cannot be set to 0'
Comment on lines +152 to +153
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer removing this, because otherwise it would make sense to perform the same check for min-spaces-after too (which isn't allowed to be zero), and probably in many other rules too.

if (conf['min-spaces-after'] > 0 and
conf['min-spaces-after'] > conf['max-spaces-after']):
return '"min-spaces-after" cannot be greater than "max-spaces-after"'


def check(conf, token, prev, next, nextnext, context):
if isinstance(token, yaml.BlockEntryToken):
problem = spaces_after(token, prev, next,
max=conf['max-spaces-after'],
max_desc='too many spaces after hyphen')
if problem is not None:
yield problem
if conf['max-spaces-after'] > 0:
problem = spaces_after(token, prev, next,
max=conf['max-spaces-after'],
max_desc='too many spaces after hyphen')
if problem is not None:
yield problem

if conf['min-spaces-after'] > 0:
Copy link
Owner

Choose a reason for hiding this comment

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

Doing the check if conf['min-spaces-after'] == 1 is useless (it cannot be lower than 1) and will degrade performance.

I propose:

Suggested change
if conf['min-spaces-after'] > 0:
if conf['min-spaces-after'] > 1:

problem = spaces_after(token, prev, next,
min=conf['min-spaces-after'],
min_desc='too few spaces after hyphen')
if problem is not None:
yield problem

if (conf['check-scalars'] and conf['min-spaces-after'] > 0
and isinstance(token, yaml.ScalarToken)):
# Token identified as a scalar so there is no space after the
# hyphen: no need to count
if token.value.startswith('-'):
yield LintProblem(
token.start_mark.line + 1,
token.start_mark.column + 1,
'too few spaces after hyphen')