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 forbid-in-block-sequences feature for detecting empty values in block sequences #604

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

ericalmeidasp
Copy link
Contributor

feat: Add forbid-in-list-items feature to detect empty values in lists

This commit introduces a new feature, forbid-in-list-items, which allows users to check for empty values within lists. The default behavior is set to false, but it can be activated by setting the forbid-in-list-items configuration variable to true.

The check function has been updated to handle this feature, and it will now identify and report empty values within list items when forbid-in-list-items is enabled.

This enhancement provides users with more control and flexibility when using the linter for YAML files.

@coveralls
Copy link

coveralls commented Nov 8, 2023

Coverage Status

coverage: 99.396% (+0.005%) from 99.391%
when pulling 62f54e2 on ericalmeidasp:master
into e1d45c3 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 Éric, and thanks for contributing to yamllint!

For consistency with YAML vocabulary and already-existing yamllint options forbid-in-block-mappings and forbid-in-flow-mappings, this new feature should be named forbid-in-block-sequences (and a new option forbid-in-flow-sequences shouldn't be necessary because I believe [1, , 3] is invalid YAML syntax). Same for the documentation paragraph, the LintProblem message and test functions names.

Since the rule empty-values is disabled in default and relaxed configurations, I would set this option enabled by default, like forbid-in-block-mappings and forbid-in-flow-mappings are. It will create new errors or warnings for some users with specific configurations, but it's easy to fix. What do you think?

tests/rules/test_empty_values.py Outdated Show resolved Hide resolved
@ericalmeidasp
Copy link
Contributor Author

Hello! Thank you so much for the code review,

In response to the valuable feedback provided, I have made the following changes:

  1. Renamed the feature from forbid-in-list-items to forbid-in-block-sequences.

  2. Updated the documentation paragraph, LintProblem message, and test function names to align with the new feature name.

  3. Set the forbid-in-block-sequences option to be enabled by default, similar to forbid-in-block-mappings and forbid-in-flow-mappings. I agree with u, adjusted to maintain consistency.

In addition to these changes, I've also added more test cases to cover various scenarios. However, due to the forbid-in-block-sequences option being set as the default, I had to modify the existing test cases by including forbid-in-block-sequences: false to avoid interference with the existing test scenarios in the test_empty_values.py file.

Thanks!

@ericalmeidasp ericalmeidasp changed the title Add forbid-in-list-items feature for detecting empty values in lists Add forbid-in-block-sequences feature for detecting empty values in block sequences Nov 9, 2023
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.

This looks perfect, thank you! I'm merging this so it goes in the next yamllint release.

⚠️ To all yamllint users: this is a BREAKING CHANGE if the empty-values rule is enabled in your configuration. But if it's the case, you probably want this new option forbid-in-block-sequences enabled too, for consistency. In case you want to disable it, just use this configuration in your .yamllint:

rules:
  empty-values:
    forbid-in-block-mappings: true
    forbid-in-flow-mappings: true
    forbid-in-block-sequences: false

@adrienverge adrienverge merged commit a26dd00 into adrienverge:master Nov 9, 2023
7 checks passed
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