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 ignore blank lines option #747

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

Agreon
Copy link
Contributor

@Agreon Agreon commented Oct 2, 2022

closes #581

@Agreon
Copy link
Contributor Author

Agreon commented Oct 2, 2022

@MitMaro I'm not sure about why the formatter changed these unrelated lines. I can fix that of course, if you say so ;)

@coveralls
Copy link

coveralls commented Oct 2, 2022

Pull Request Test Coverage Report for Build 3191134991

  • 6 of 6 (100.0%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 95.275%

Files with Coverage Reduction New Missed Lines %
src/view/src/lib.rs 1 95.96%
Totals Coverage Status
Change from base Build 3174270206: -0.02%
Covered Lines: 4033
Relevant Lines: 4233

💛 - Coveralls

@MitMaro
Copy link
Owner

MitMaro commented Oct 2, 2022

@MitMaro I'm not sure about why the formatter changed these unrelated lines. I can fix that of course, if you say so ;)

Thanks for the PR! Looks good, besides the random formatting problems. That might be because of running stable vs nightly Rust. Sadly, rustfmt has a number of things that don't work on stable.

For testing, if you run cargo make dev (install cargo make: cargo install --force cargo-make), it will run through all the same steps that CI runs. It will ensure that the various steps are run on stable/nightly as needed. Just a note, it will update your stable and nightly Rust versions.

@MitMaro
Copy link
Owner

MitMaro commented Oct 2, 2022

Also, there are some lint failures that are not related to your PR. I will take care of those shortly, to get them out of your way. :)

@MitMaro MitMaro self-requested a review October 2, 2022 14:06
@MitMaro
Copy link
Owner

MitMaro commented Oct 3, 2022

Looks like there is one last formatting issue, but otherwise looks good.

Can you rebase and squash your commits together along with the remaining formatting issue? That should fix the failing linting issues as well.

Otherwise, I looked at the change, and it looks good. Thanks for the doc and test updated!

@Agreon Agreon force-pushed the add-diff-ignore-blank-lines-option branch from 29bcf65 to 1b6e5f2 Compare October 3, 2022 20:42
@Agreon
Copy link
Contributor Author

Agreon commented Oct 3, 2022

@MitMaro Good thing i have a tool for that ;). With regard to the formatting: It seems like this one error comes from a unrelated library?

@MitMaro
Copy link
Owner

MitMaro commented Oct 5, 2022

@MitMaro Good thing i have a tool for that ;).

I had a genuine chuckle at that one! 😆

With regard to the formatting: It seems like this one error comes from a unrelated library?

You are correct that it's coming from a seemingly unrelated library. There is a bug in rustfmt that causes rustfmt to panic/crash when it encounters a long line. The crash is partially caused by a bug in annotate-snippets-rs.

What is happening in this PR, is that the #[case...] lines are too long, and it causes rustfmt to panic when it reaches those lines. Sadly, this also means that they need to be manually formatted, and rustfmt can't fix the problem.

@Agreon Agreon force-pushed the add-diff-ignore-blank-lines-option branch from 1b6e5f2 to 354b513 Compare October 5, 2022 16:26
@MitMaro
Copy link
Owner

MitMaro commented Oct 6, 2022

Thanks for implementing the new option and putting in the effort to get things through the formatter!

I'm uncertain if you're participating in Hacktoberfest, but if you are, the PR should be included in your contributions. If not, please let me know, and I will make sure it is.

@MitMaro MitMaro merged commit 9652272 into MitMaro:master Oct 6, 2022
@MitMaro MitMaro self-assigned this Oct 6, 2022
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.

Support "ignore blank lines" in diff
3 participants