8000 feat: consolidate adjacent error messages by klaernie · Pull Request #360 · editorconfig-checker/editorconfig-checker · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: consolidate adjacent error messages #360

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Sep 4, 2024
Merged

feat: consolidate adjacent error messages #360

merged 21 commits into from
Sep 4, 2024

Conversation

klaernie
Copy link
Member

This changes the human output format to collect multiple identical error
messages on consecutive lines into one error message that list both the
starting and ending line.

This is still missing a properly written testcase, I so far only tested
with

make
(cd testfiles; ../bin/ec multiple-wrong-indentations.txt)

So far the output for the newly included testfile is this:

multiple-wrong-indentations.txt:
        2-4: Wrong amount of left-padding spaces(want multiple of 10)
        8-9: Wrong amount of left-padding spaces(want multiple of 10)
6 errors found
1 files checked

However there are still a few things that annoy me:

  1. The summary of how many errors are present is now incorrect, since the error.GetErrorCount() is called by main(), before any consolidation happened.
    However one should not hoist the consolidation up into main(), since it must only happen for the human output (or the eventual github action format mentioned in More Output Formats #245).
    My first instinct would be to let error.PrintErrors() handle writing that summary as well, but technically that would add a side effect for error.TestPrintErrors().

  2. I have no idea how to work with golang's testing infrastructure. I'd roughly guess that one would have to make PrintErrors() side effect free by collecting the lines to be printed and returning them, letting the caller handle the actual printing, but being then able to validate the output lines in a testcase.

I'd be really happy for any feedback, while programming for years, this is maybe my third time touching golang - which is quite a different beast compared to Perl. So especially if any I did is not idiomatic or hard to understand, please tell me! (aka: perlcritic --brutal)

@mstruebing
Copy link
Member

This looks like a great PR already.
I will need to find some time to look over the changes and think about it.
If someone other maintainer has time to have a look that would be very much appreciated 🫶

CI is failing because of your commit message, we require semantic commits i.e. feat: blabla.
I'm not entirely sure we use that still anywhere, so we might be able to remove that check but if we want to move further with automatic releasing in the future it could help to have semantic commits.

@klaernie
Copy link
Member Author

that's a really easy fix at least - I fixed the commit messages, treating the change to the human output as a breaking change, since somebody out there is probably going to rely on it. Sadly I didn't see a CONTRIBUTING.md or similar section in the README and didn't notice in the existing commits.

Since it didn't seem much work anymore, I also implemented the output format that github actions uses.

@codecov Codecov
Copy link
codecov bot commented Aug 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.04%. Comparing base (d6c17be) to head (bb077d7).
Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
pkg/error/error.go 98.07% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #360      +/-   ##
==========================================
+ Coverage   84.89%   87.04%   +2.15%     
==========================================
  Files           8        8              
  Lines         695      610      -85     
==========================================
- Hits          590      531      -59     
+ Misses         81       55      -26     
  Partials       24       24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@klaernie
Copy link
Member Author

I now managed to build a test case for the newly introduced error.ConsolidateErrors() function - hopefully next time Codecov runs this should bring the test coverage back up again.

ccoVeille
ccoVeille requested changes Aug 16, 2024
View reviewed changes
Copy link
Contributor
@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

Typo spotted

@klaernie
Copy link
Member Author
klaernie commented Aug 16, 2024 •
edited
Loading

of cause now testing the tester was needed as well - that finally should bring testing coverage up to where it was before I started this branch

@klaernie
Copy link
Member Author

okay, now the ci should pass - turns out I had a unicode nonbreaking space in the second to last commit, and the wording in the last commit tripped the "subject must not be sentence-case" rule. Now I verified locally to ensure the commit messages pass.

ccoVeille
ccoVeille reviewed Aug 17, 2024
View reviewed changes
pkg/error/error_test.go Outdated

expected := []ValidationError{
{LineNumber: -1, Message: errors.New("file-level error")},
{LineNumber: 1, ConsecutiveCount: 1, Message: errors.New("message kind one")},
Copy link
Contributor

Choose a reason for hiding this comment

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

I know lists start at 0

But I would have expected to find a Consecutive count of 2.

With the current example and code, line 1 will report a consecutive count of 1.

A human would expect to know that the error reported line 1 was reported twice, so 2

Copy link
Member Author

Choose a reason for hiding this comment

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

hm.. ConsecutiveCount is the count of additional error messages that followed the first one - so it excludes the first one (meaning ConsecutiveCount == 0 is "one singular message").
But switching to storing according to your model would mean the last-line-of-block calculation becomes a tad simpler. I'll think about that one - maybe renaming the property to a better name is better to make it clearer (I'm thinking about AdditionalIdenticalErrorCount for example)

Copy link
Contributor

Choose a reason for hiding this comment

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

It would make sense.

I'm not good with wording, but yes I think it would make it easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed it, sadly I had no better idea than AdditionalIdenticalErrorCount, so that one it is for now ;)