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

Adjustable whitelines feature #244

Merged
merged 6 commits into from
May 26, 2023

Conversation

Klavionik
Copy link
Contributor

I've been suffering from the same behavior that was mentioned in several issues:

So I decided to give it a try and implement a solution.

What I did was:

  1. I found a piece of code that removes our beloved whitelines (namely the _fix_whitelines method) and added a new config option, whitelines, to make it adjustable (right now it just multiplies whitelines by zero, removing them completely).
  2. I rewrote _fix_flow_style_lists method so that it not only fixes Ruyaml's bad behavior, but also keeps the whitelines that happened to be inside the flow style list. I also made it run before _fix_whitelines. Then I had to fix test_fix_code_functions_emit_debug_logs as it was comparing lists and broke after I changed the order of fixers (maybe I should've just reorder the expected list?).
  3. I slightly fixed _add_newline_at_end_of_file so that it won't add excessive whitelines, only make sure that there's exactly 1 newline at the end of a file.

This is probably not the ideal solution, but I hope it helps to cover some common use cases.

Checklist

  • Add test cases to all the changes you introduce
  • Update the documentation for the changes

@coveralls
Copy link

coveralls commented May 20, 2023

Pull Request Test Coverage Report for Build 5032706547

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.005%) to 99.793%

Totals Coverage Status
Change from base Build 4881195162: -0.005%
Covered Lines: 482
Relevant Lines: 483

💛 - Coveralls

Copy link
Owner

@lyz-code lyz-code left a comment

Choose a reason for hiding this comment

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

Hi @Klavionik thank you so much for your pull request, it is beautiful. You not only fixed one of the most complained issues, but also reduced the complexity of the code and added more comments.

Sorry for taking so long to answer, please take a look at the only comment I've made and we'll be ready to merge it :)

src/yamlfix/adapters.py Outdated Show resolved Hide resolved
@calumy
Copy link

calumy commented May 24, 2023

Would you consider adding this to the CLI too, rather than relying on an environment variable? This would be useful when running with pre-commit.

@lyz-code
Copy link
Owner

By the way this PR will probably solve also this issue #237. Can you please check if it does? If you can't it's fine

@lyz-code
Copy link
Owner

Would you consider adding this to the CLI too, rather than relying on an environment variable? This would be useful when running with pre-commit.

@calumy can you try to use a config file with the -c flag?

@Klavionik
Copy link
Contributor Author

By the way this PR will probably solve also this issue #237. Can you please check if it does? If you can't it's fine

This PR indeed solves this problem.

Before:

test: |
  Onetwothree  

  Onetwothree

  Onetwothree

After (with whitelines = 1)

---
test: |-
  Onetwothree  

  Onetwothree

  Onetwothree

@Klavionik
Copy link
Contributor Author

Thank you! I'm glad to help. Actually 4 days is a pretty fast response, so don't be hard on youself!

I acted upon your review and force-pushed the changes.

@calumy
Copy link

calumy commented May 24, 2023

Would you consider adding this to the CLI too, rather than relying on an environment variable? This would be useful when running with pre-commit.

@calumy can you try to use a config file with the -c flag?

Apologies for the noise. I forgot that it was possible to configure through a config file. Thanks for highlighting this fact and thanks for the great tool.

Copy link
Owner

@lyz-code lyz-code 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 your contribution and kind words ^^

@lyz-code lyz-code merged commit 29e113c into lyz-code:main May 26, 2023
5 checks passed
@lyz-code
Copy link
Owner

Included in 1.10.0

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.

None yet

4 participants