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

rule new-lines: add type: platform configuration option #474

Merged
merged 3 commits into from
Jun 23, 2022
Merged

rule new-lines: add type: platform configuration option #474

merged 3 commits into from
Jun 23, 2022

Conversation

Cube707
Copy link
Contributor

@Cube707 Cube707 commented Jun 18, 2022

This PR follows up on #456 in which it was concluded that the default for the new-lines rule should not be changed but rather a new option is to be introduced.

Problem

As explained in this comment on the parent PR yamllint currently has portability issues that make it hard to use on Windows clients. These stem from git converting line-endings to the platform default on checkout and yamllint assuming that the line-ending type should be enforced for all development platforms. This leads to windows-clients needing to be heavily reconfigured to make yamllint work there properly.

PR's content

This PR introduces a new option for the new-lines rule: type: auto.

The new option infers the correct newline character(s) from the operating system that is currently running yamllint. This allows developers to create yaml-files with their systems default settings and relying on git to convert them.

Projects that wish to enforce a certain type of line-ending are still able to do so by keeping one of the original two options.

Linting / Testing

flake8, doc8 and unittest completet without error on my local machine.

@Cube707 Cube707 changed the title rule new-lines: add type: auto configuration option rule new-lines: add type: auto configuration option Jun 20, 2022
@coveralls
Copy link

coveralls commented Jun 20, 2022

Coverage Status

Coverage increased (+0.003%) to 99.15% when pulling 1d958a9 on Cube707:platform-newlines into 695fc5f 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.

@Cube707 thanks for this PR!

I still think type: platform is better than type: auto, because auto relates to auto-detection from the file in other parts of yamllint. On the other hand, platform causes no ambiguity.

Please see the lint results, there are some errors.

You can remove rule from the commit title: rule new-lines: ..., for consistency with other commits.

About the first commit explicitly check \n for type: unix, I don't understand it, and I'm not sure what kind of side-effects it could have. Please check out bullet 6 of the contributing guidelines: anyone should be able to quickly read and understand the reason for a change (in case it induces problems on some platforms, for example...)

yamllint/rules/new_lines.py Outdated Show resolved Hide resolved
yamllint/rules/new_lines.py Outdated Show resolved Hide resolved
yamllint/rules/new_lines.py Outdated Show resolved Hide resolved
tests/rules/test_new_lines.py Outdated Show resolved Hide resolved
tests/rules/test_new_lines.py Show resolved Hide resolved
tests/rules/test_new_lines.py Outdated Show resolved Hide resolved
yamllint/rules/new_lines.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Cube707 Cube707 left a comment

Choose a reason for hiding this comment

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

Is the commit history so acceptable? Or should I still remove the explicitly check \n for type: unix commit? (or explain it better?)

Are you OK with having the refactoring as a separate commit? I expect this would make it easier to find the source of problems between the new option and the refactoring. But I can also make it a single commit.

Thanks for spending so much time on this!

EDIT: I will also rebase after the comments below are resolved.

tests/rules/test_new_lines.py Outdated Show resolved Hide resolved
tests/rules/test_new_lines.py Outdated Show resolved Hide resolved
yamllint/rules/new_lines.py Outdated Show resolved Hide resolved
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.

Thanks @Cube707!

The history looks good but I would prefer:
- either merging the 2 last commits (refactor + add 'platform'),
- or applying the refactor before the 'platform' addition,
in order to reduce the diff and the hassle of git blame.

tests/rules/test_new_lines.py Outdated Show resolved Hide resolved
tests/rules/test_new_lines.py Outdated Show resolved Hide resolved
yamllint/rules/new_lines.py Outdated Show resolved Hide resolved
tests/rules/test_new_lines.py Outdated Show resolved Hide resolved
@Cube707
Copy link
Contributor Author

Cube707 commented Jun 21, 2022

should be good now.

unittest and flake8 pass on my machine.

@Cube707 Cube707 requested a review from adrienverge June 21, 2022 16:55
@adrienverge
Copy link
Owner

Looks good to me, but the linter complains: you need to add an extra line before the new import from os import linesep.

yamllint/rules/new_lines.py:docstring of yamllint.rules.new_lines:9:Bullet list ends without a blank line; unexpected unindent.

@Cube707
Copy link
Contributor Author

Cube707 commented Jun 22, 2022

found the problem. There was a \n in the docstring that was not properly escaped. (See compare)

python setup.py build_sphinx complets now on my machine.

Cube707 added 3 commits June 22, 2022 20:03
To be more consistent with the other types, unix now also checks against
the expected newline character (`\n`) instead of checking if a wrong
character (`\r`) is present
Both options where using identical code.  Now the newline character
is determined beforehand depending on the selected option and then
the same code can be used for all options
The new option infers the correct newline character(s) from the
operating system running yamllint.
@Cube707
Copy link
Contributor Author

Cube707 commented Jun 22, 2022

rebased onto master

@Cube707 Cube707 changed the title rule new-lines: add type: auto configuration option rule new-lines: add type: platform configuration option Jun 22, 2022
@adrienverge
Copy link
Owner

found the problem. There was a \n in the docstring that was not properly escaped. (See compare)

Well seen.

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.

Thanks for this work Jan! 👍

@adrienverge adrienverge merged commit 7d9c824 into adrienverge:master Jun 23, 2022
@Cube707 Cube707 deleted the platform-newlines branch June 23, 2022 07:46
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