Ensure that single space between tasks is preserved when using --write #3641

shatakshiiii · 2023-08-01T09:09:34Z

src/ansiblelint/yaml_utils.py

ssbarnea · 2023-08-07T12:56:24Z

test/test_yaml_utils.py

+
+    before_formatting = yaml.loads(task_file_content)
+    dump_from_formatting = yaml.dumps(before_formatting)
+    assert task_file_content == dump_from_formatting


Basically you do only test that the formatter does not change the output in any way, but that is not a good enough way to test that the spacing is the correct one. I would want to also see a case where it is bad before and transformer is changing it the way we want.

Another question that I have: if I comment the code change and only include the tests, are they going to pass? If they pass already it means they have only very limited value. If not, they are likely ok.

I see that the body of the test looks identical. Maybe it would be better to use parameterized tests instead of repeating the same body of the test. Also this should make it easier to add other test files later, for other test-cases.

Basically you do only test that the formatter does not change the output in any way, but that is not a good enough way to test that the spacing is the correct one. I would want to also see a case where it is bad before and transformer is changing it the way we want.

Initially I was testing exacting like this (with two different files basically) one with bad/extra spacing and other one with correct spacing after transform. And it was working as we want, but after pushing that commit prettier hook in the pre-commit modified the before-file to remove some of the extra spaces between the tasks so I switched to using a single file because eventually pre-commit was making both of them identical.

In this commit: 2ddcea0

Update: The problem with prettier hook got resolved after moving these tests into test/fixtures/formatting-before/ directory.

Another question that I have: if I comment the code change and only include the tests, are they going to pass? If they pass already it means they have only very limited value. If not, they are likely ok.

These new added tests won't pass, if I comment the code changes.

ssbarnea · 2023-08-07T13:01:59Z

test/test_yaml_utils.py

+
+    before_formatting = yaml.loads(task_file_content)
+    dump_from_formatting = yaml.dumps(before_formatting)
+    assert task_file_content == dump_from_formatting


Another question that I have: if I comment the code change and only include the tests, are they going to pass? If they pass already it means they have only very limited value. If not, they are likely ok.

ssbarnea · 2023-08-07T13:02:53Z

test/test_yaml_utils.py

+
+    before_formatting = yaml.loads(task_file_content)
+    dump_from_formatting = yaml.dumps(before_formatting)
+    assert task_file_content == dump_from_formatting


I see that the body of the test looks identical. Maybe it would be better to use parameterized tests instead of repeating the same body of the test. Also this should make it easier to add other test files later, for other test-cases.

for more information, see https://pre-commit.ci

…r formatting

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [ansible-lint](https://github.com/ansible/ansible-lint) ([changelog](https://github.com/ansible/ansible-lint/releases)) | minor | `==6.17.2` -> `==6.18.0` | --- ### Release Notes <details> <summary>ansible/ansible-lint (ansible-lint)</summary> ### [`v6.18.0`](https://github.com/ansible/ansible-lint/releases/tag/v6.18.0) [Compare Source](ansible/ansible-lint@v6.17.2...v6.18.0) #### Minor Changes - Limit the maximum block depth ([#3602](ansible/ansible-lint#3602)) [@Ruchip16](https://github.com/Ruchip16) - Transform functionality for command_instead_of_shell ([#3675](ansible/ansible-lint#3675)) [@ajinkyau](https://github.com/ajinkyau) - Add rule to check the number of tasks ([#3156](ansible/ansible-lint#3156)) [@Ruchip16](https://github.com/Ruchip16) #### Bugfixes - Clarify loop-var-prefix rule and code snippet ([#3642](ansible/ansible-lint#3642)) [@schwarmco](https://github.com/schwarmco) - Update `version_added` for complexity rule ([#3623](ansible/ansible-lint#3623)) [@ajinkyau](https://github.com/ajinkyau) - Ensure that single space between tasks is preserved when using --write ([#3641](ansible/ansible-lint#3641)) [@shatakshiiii](https://github.com/shatakshiiii) - Update ansible-compat used for testing ([#3664](ansible/ansible-lint#3664)) [@ssbarnea](https://github.com/ssbarnea) - Document `yaml[line-length]` rule ([#3653](ansible/ansible-lint#3653)) [@shatakshiiii](https://github.com/shatakshiiii) - Prevent use of spdx-tools 0.8.0 due to breaking changes ([#3649](ansible/ansible-lint#3649)) [@ssbarnea](https://github.com/ssbarnea) - fixes dead marketplace link ([#3631](ansible/ansible-lint#3631)) [@wookietreiber](https://github.com/wookietreiber) - Improve profile information on summary line ([#3637](ansible/ansible-lint#3637)) [@ziegenberg](https://github.com/ziegenberg) - command-instead-of-module: allow `git rev-parse` ([#3610](ansible/ansible-lint#3610)) [@JohnVillalovos](https://github.com/JohnVillalovos) - Include filepaths starting from $HOME in lintables ([#3621](ansible/ansible-lint#3621)) [@shatakshiiii](https://github.com/shatakshiiii) - Update \_mockings.py to fix bug created in [#3390](ansible/ansible-lint#3390) ([#3614](ansible/ansible-lint#3614)) [@karcaw](https://github.com/karcaw) - Allow to set gather_facts as templated boolean ([#3606](ansible/ansible-lint#3606)) [@noonedeadpunk](https://github.com/noonedeadpunk) - Add dependency version check for collection metadata ([#3601](ansible/ansible-lint#3601)) [@ajinkyau](https://github.com/ajinkyau) - Fix installation of dependencies when run as an action ([#3592](ansible/ansible-lint#3592)) [@ssbarnea](https://github.com/ssbarnea) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.home/nrdufour/home-ops/pulls/49 Co-authored-by: Renovate <renovate@ptinem.io> Co-committed-by: Renovate <renovate@ptinem.io>

github-actions bot added the bug label Aug 1, 2023

shatakshiiii changed the title ~~Ensure that single space between tasks are preserved when using --write~~ Ensure that single space between tasks is preserved when using --write Aug 1, 2023

shatakshiiii marked this pull request as ready for review August 4, 2023 09:42

shatakshiiii requested a review from a team as a code owner August 4, 2023 09:42

shatakshiiii requested review from a team, audgirka, priyamsahoo and Ruchip16 August 4, 2023 09:42

cognifloyd reviewed Aug 4, 2023

View reviewed changes

src/ansiblelint/yaml_utils.py Show resolved Hide resolved

ssbarnea reviewed Aug 7, 2023

View reviewed changes

ssbarnea requested changes Aug 7, 2023

View reviewed changes

shatakshiiii force-pushed the fix/transform branch 2 times, most recently from 158a3c5 to 4652da5 Compare August 8, 2023 13:57

shatakshiiii requested review from ssbarnea and ganeshrn August 8, 2023 14:40

ssbarnea self-assigned this Aug 10, 2023

ganeshrn approved these changes Aug 10, 2023

View reviewed changes

shatakshiiii and others added 10 commits August 21, 2023 11:03

Ensure that single space between tasks are preserved when using --write

35016e7

Fix for test_formatted_yaml_loader_dumper

36bbe08

Add tests for formatting task files

bf5449b

chore: auto fixes from pre-commit.com hooks

2305c55

for more information, see https://pre-commit.ci

Fix the tests and add more description

597ef3e

Change test_formatted_task to check single task file, before and afte…

72975d7

…r formatting

Add test for playbook scenario

8e9e848

Changes in the test

93eb0cb

Improvements in the tests

b281894

Simplify test

ebb7e13

ssbarnea force-pushed the fix/transform branch from b650f8c to ebb7e13 Compare August 21, 2023 10:15

ssbarnea approved these changes Aug 21, 2023

View reviewed changes

ssbarnea merged commit 9021516 into ansible:main Aug 21, 2023

shatakshiiii deleted the fix/transform branch August 21, 2023 10:29

shatakshiiii mentioned this pull request Aug 21, 2023

Spaces between tasks in block not preserved when writing yaml #2112

Closed

cognifloyd approved these changes Sep 6, 2023

View reviewed changes

shatakshiiii self-assigned this Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that single space between tasks is preserved when using --write #3641

Ensure that single space between tasks is preserved when using --write #3641

shatakshiiii commented Aug 1, 2023

ssbarnea Aug 7, 2023

ssbarnea Aug 7, 2023

ssbarnea Aug 7, 2023

shatakshiiii Aug 7, 2023 •

edited

Loading

shatakshiiii Aug 7, 2023 •

edited

Loading

ssbarnea Aug 7, 2023

ssbarnea Aug 7, 2023

Ensure that single space between tasks is preserved when using --write #3641

Ensure that single space between tasks is preserved when using --write #3641

Conversation

shatakshiiii commented Aug 1, 2023

ssbarnea Aug 7, 2023

Choose a reason for hiding this comment

ssbarnea Aug 7, 2023

Choose a reason for hiding this comment

ssbarnea Aug 7, 2023

Choose a reason for hiding this comment

shatakshiiii Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

shatakshiiii Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

ssbarnea Aug 7, 2023

Choose a reason for hiding this comment

ssbarnea Aug 7, 2023

Choose a reason for hiding this comment

shatakshiiii Aug 7, 2023 •

edited

Loading

shatakshiiii Aug 7, 2023 •

edited

Loading