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

Configure path prefix via processor abstraction #1226

Merged
merged 5 commits into from Jul 10, 2020
Merged

Configure path prefix via processor abstraction #1226

merged 5 commits into from Jul 10, 2020

Conversation

jwilner
Copy link
Contributor

@jwilner jwilner commented Jul 10, 2020

Meant as an alternative to #1225 which uses the standard processor abstraction

problem

It's often convenient in monorepos to run a tool from a different directory but still report issues relative to the root of the repository.

This is exactly the problem in the github action golangci/golangci-lint-action#31

proposed solution

Add a flag for specifying a path prefix in output. Default behavior remains the same.

Thanks for a great tool!

@sayboras sayboras mentioned this pull request Jul 10, 2020
Closed
jwilner
jwilner commented Jul 10, 2020
View reviewed changes
Copy link
Contributor Author

@jwilner jwilner left a comment •
edited

Choose a reason for hiding this comment

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

Well these checks are just wrong (the scope lint ones)

@jwilner
Copy link
Contributor Author

jwilner commented Jul 10, 2020

Will squash merge away noise

@SVilgelm
Copy link
Member

Will squash merge away noise

no worries, it's only one allowed option to merge any pull requests here.

@SVilgelm
Copy link
Member

Since it adds a new config option, could you also add a test in the test/run_test.go?

@SVilgelm SVilgelm mentioned this pull request Jul 10, 2020
Closed
sayboras
sayboras reviewed Jul 10, 2020
View reviewed changes
pkg/result/processors/path_prefixer.go

// Process adds the prefix to each path
func (p *PathPrefixer) Process(issues []result.Issue) ([]result.Issue, error) {
if p.prefix != "" {
Copy link
Member

Choose a reason for hiding this comment

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

The current logic seems fine to me.

Just a note on immutability, I just take a look at existing implementation of processors. Seems like https://github.com/golangci/golangci-lint/blob/master/pkg/result/processors/utils.go#L40 is widely used (this function create a copy of slides).

What do you think ?

Copy link
Contributor Author

@jwilner jwilner Jul 10, 2020 •
edited

Choose a reason for hiding this comment

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

Well, it's safe for this to be mutating the passed in slice because, by definition, the processors happen serially, so there's no risk of a race on the mutated memory.

I would guess the transformSlices function exists because it's easy to mess up working with a slice of values -- every value in a range loop is a copy, etc, etc.

As it is, I figured this is a memory-sensitive application and the Issue struct is non-trivial in its size, so I'd avoid the unnecessary copies and alloc.

Without running any sort of measurements, I'm guessing there's a lot of fat around this processor chain's memory usage and shifting to a pattern like that I've used here would cut it down a bunch.

@sayboras sayboras requested a review from a team July 10, 2020 04:35
@sayboras sayboras requested a review from a team July 10, 2020 10:56
@jwilner
Copy link
Contributor Author

jwilner commented Jul 10, 2020

Will squash merge away noise

no worries, it's only one allowed option to merge any pull requests here.

The only civilized option! :)

@jwilner
Copy link
Contributor Author

jwilner commented Jul 10, 2020

@SVilgelm added to run_test.go per your request.

I'm impressed with this project's test coverage. 👍

@jwilner jwilner mentioned this pull request Jul 10, 2020
Merged
SVilgelm
SVilgelm approved these changes Jul 10, 2020
View reviewed changes
Copy link
Member

@SVilgelm SVilgelm left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good to me.

@jwilner
Copy link
Contributor Author

jwilner commented Jul 10, 2020

🥳 looking forward to using it :)

@SVilgelm SVilgelm changed the title Alternative: Configure path prefix via processor abstraction Configure path prefix via processor abstraction Jul 10, 2020
@jwilner
Copy link
Contributor Author

jwilner commented Jul 10, 2020

Oh I don't have the ability to merge btw, if that was ambiguous.

@SVilgelm
Copy link
Member

Let's wait a feedback from @sayboras, he also revised this PR

@SVilgelm SVilgelm requested review from sayboras and a team July 10, 2020 18:05
sayboras
sayboras approved these changes Jul 10, 2020
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sayboras sayboras merged commit 6550984 into golangci:master Jul 10, 2020
@golangci-automator
Copy link

Hey, @jwilner — we just merged your PR to golangci-lint! 🔥🚀

golangci-lint is built by awesome people like you. Let us say “thanks”: we just invited you to join the GolangCI organization on GitHub.
This will add you to our team of maintainers. Accept the invite by visiting this link.

By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.
More information about contributing is here.

Thanks again!

@sayboras
Copy link
Member

It's wierd that test failed in master https://github.com/golangci/golangci-lint/runs/859811449 and another one #1201

@jwilner Do you have any idea or pointer ?

@jwilner
Copy link
Contributor Author

jwilner commented Jul 11, 2020

@sayboras whoa, huh, one of the tests I added!? I'd be happy to look -- but I can't tell which test it is from the output. Can you point me in the right direction?

@SVilgelm
Copy link
Member

@jwilner Here it is: https://github.com/golangci/golangci-lint/runs/859880992?check_suite_focus=true#step:5:881

@jwilner
Copy link
Contributor Author

jwilner commented Jul 11, 2020

So something about the test runner is failing on the command

../golangci-lint run --allow-parallel-runners --path-prefix=cool testdata/withTests

Isn't the test dir testdata/withTests used within other tests? Hmmm. Digging deeper.

=== RUN   TestPathPrefix/prefixed
level=info msg="[test] stderr: "
level=info msg="[test] ran [../golangci-lint run --allow-parallel-runners --path-prefix=cool testdata/withTests] in 358.830342ms"
    TestPathPrefix/prefixed: testshared.go:71: 
        	Error Trace:	testshared.go:71
        	            				run_test.go:308
        	Error:      	Expect "level=error msg="Running error: context loading failed: failed to load packages: cannot find package \".\" in:\n\t/home/runner/work/golangci-lint/golangci-lint/test/testdata/withTests: failed to analyze"
        	            	" to match "^cool/testdata/withTests"
        	Test:       	TestPathPrefix/prefixed
        	Messages:   	exit code is 3

@jwilner jwilner mentioned this pull request Jul 11, 2020
Merged
@jwilner
Copy link
Contributor Author

jwilner commented Jul 11, 2020

@SVilgelm @sayboras I've opened up a fix here if you want to check it out: #1229

@SVilgelm
Copy link
Member

@jwilner thank you, I merged it to unblock the CI

This was referenced Dec 19, 2020
Closed
Closed
Closed
Closed
@ldez ldez added this to the v1.28 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SVilgelm SVilgelm SVilgelm approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jwilner @SVilgelm @sayboras @ldez