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.

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

feat: add support for multiple errors per file. #357

Merged
merged 23 commits into from May 30, 2022
Merged

feat: add support for multiple errors per file. #357

merged 23 commits into from May 30, 2022

Conversation

i4ki
Copy link
Contributor

@i4ki i4ki commented May 24, 2022 •
edited

This PR adds support for multiple errors per configuration file.

@i4ki i4ki marked this pull request as draft May 24, 2022 23:04
@codecov
Copy link

codecov bot commented May 25, 2022 •
edited

Codecov Report

Merging #357 (d5d849c) into main (c6999dd) will increase coverage by 0.23%.
The diff coverage is 80.50%.

@@            Coverage Diff             @@
##             main     #357      +/-   ##
==========================================
+ Coverage   67.79%   68.02%   +0.23%     
==========================================
  Files          37       37              
  Lines        6844     6947     +103     
==========================================
+ Hits         4640     4726      +86     
- Misses       1950     1967      +17     
  Partials      254      254
Flag Coverage Δ
tests 68.02% <80.50%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
errors/error.go 93.36% <76.36%> (-5.55%) ⬇️
hcl/hcl.go 87.47% <80.72%> (+1.17%) ⬆️
errors/list.go 92.40% <86.66%> (-7.60%) ⬇️
git/git.go 74.16% <0.00%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6999dd...d5d849c. Read the comment docs.

@i4ki i4ki marked this pull request as ready for review May 25, 2022 12:50
@i4ki i4ki requested a review from katcipis May 25, 2022 12:50
i4ki
i4ki commented May 25, 2022
View reviewed changes
errors/list.go Outdated Show resolved Hide resolved
This was referenced May 25, 2022
Merged
Merged
katcipis
katcipis previously approved these changes May 25, 2022
View reviewed changes
Copy link
Contributor

@katcipis katcipis left a comment

Choose a reason for hiding this comment

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

LGTM

My main source of confusion was dropping errors.As usage, but I imagine I'm missing something and it is not critical to the PR.

Also we are missing some tests involving multiple errors, but we can improve that later too.

errors/error.go Outdated Show resolved Hide resolved
errors/error.go Outdated Show resolved Hide resolved
errors/error.go Show resolved Hide resolved
errors/list.go Outdated Show resolved Hide resolved
errors/list_test.go Outdated Show resolved Hide resolved
errors/list_test.go Outdated Show resolved Hide resolved
errors/list.go Show resolved Hide resolved
hcl/hcl.go Show resolved Hide resolved
errors/error.go Outdated Show resolved Hide resolved
errors/error.go Outdated Show resolved Hide resolved
i4ki and others added 6 commits May 27, 2022 12:05
@i4ki
improve docs
70452c7
@i4ki
improve naming
5590dcd
@i4ki @katcipis
Update errors/list_test.go
47dfdb8 
Co-authored-by: Tiago Cesar Katcipis <tiagokatcipis@gmail.com>
@i4ki @katcipis
Update errors/list_test.go
02a8029 
Co-authored-by: Tiago Cesar Katcipis <tiagokatcipis@gmail.com>
@i4ki
Merge branch 'main' of github.com:mineiros-io/terramate into i4k-add-…
af32483 
…multiple-errs
@i4ki
Merge branch 'i4k-add-multiple-errs' of github.com:mineiros-io/terram…
cba3ea0 
…ate into i4k-add-multiple-errs
@i4ki
Copy link
Contributor Author

i4ki commented May 27, 2022

@katcipis I added tests for multiple errors in the terramate-lsp PR because I thought it'd be a huge change in an already big PR, but actually it doesn't seem like it's going to be that bad.
I will at least add here the same tests I did there.

@i4ki i4ki requested review from katcipis and mariux and removed request for mariux May 27, 2022 15:32
@katcipis
Copy link
Contributor

@katcipis I added tests for multiple errors in the terramate-lsp PR because I thought it'd be a huge change in an already big PR, but actually it doesn't seem like it's going to be that bad. I will at least add here the same tests I did there.

Always like smaller PRs, thanks for doing it ❤️

katcipis
katcipis approved these changes May 30, 2022
View reviewed changes
Copy link
Contributor

@katcipis katcipis left a comment

Choose a reason for hiding this comment

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

🚀

hcl/hcl_test.go
@@ -1097,6 +1145,9 @@ func testParser(t *testing.T, tc testcase) {

// some helpers to easy build file ranges.
func mkrange(start, end hhcl.Pos) hhcl.Range {
if start.Byte == end.Byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I had some troubles with that but I missed the opportunity to improve this ❤️

@i4ki i4ki merged commit 07c9a37 into main May 30, 2022
@i4ki i4ki deleted the i4k-add-multiple-errs branch May 30, 2022 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katcipis katcipis katcipis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@i4ki @katcipis