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 GoImports Post Processor #96

Merged
merged 19 commits into from
Sep 20, 2022

Conversation

asahasrabuddhe
Copy link
Contributor

Thanks a lot for this wonderful framework. I was building a protoc-gen plugin and needed a post processor for GoImports. As it was not a part of this framework, I implemented my own. I think that this could be a useful part of this framework.

@upils
Copy link

upils commented Mar 7, 2022

@dkunitsk any chance to see this merged soon? We would also be interested in this feature.

@upils
Copy link

upils commented Mar 9, 2022

In the Match() method, setting g.filename is ineffective since the method is not working on a pointer.

But this is never use by Process() so we rewrote it this way:

type goImports struct {}

// GoImports returns a PostProcessor that run goimports on any files ending in ".go"
func GoImports() pgs.PostProcessor { return goImports{} }

func (g goImports) Match(a pgs.Artifact) bool {
	var n string

	switch a := a.(type) {
	case pgs.GeneratorFile:
		n = a.Name
	case pgs.GeneratorTemplateFile:
		n = a.Name
	case pgs.CustomFile:
		n = a.Name
	case pgs.CustomTemplateFile:
		n = a.Name
	default:
		return false
	}

	return strings.HasSuffix(n, ".go")
}

func (g goImports) Process(in []byte) ([]byte, error) {
	// We do not want to give a filename here, ever
	return imports.Process("", in, nil)
}

apply @upils's feedback!
@asahasrabuddhe
Copy link
Contributor Author

thanks @upils ! I have updated my branch with your feedback.

@keith @ardakuyumcu @pdecks Could you please review this? Thanks!

lang/go/goimports_test.go Outdated Show resolved Hide resolved
asahasrabuddhe and others added 8 commits August 9, 2022 19:31
Co-authored-by: Tricia Decker <1440268+pdecks@users.noreply.github.com>
Co-authored-by: Tricia Decker <1440268+pdecks@users.noreply.github.com>
Co-authored-by: Tricia Decker <1440268+pdecks@users.noreply.github.com>
Co-authored-by: Tricia Decker <1440268+pdecks@users.noreply.github.com>
Co-authored-by: Tricia Decker <1440268+pdecks@users.noreply.github.com>
Co-authored-by: Tricia Decker <1440268+pdecks@users.noreply.github.com>
Co-authored-by: Tricia Decker <1440268+pdecks@users.noreply.github.com>
@asahasrabuddhe
Copy link
Contributor Author

Hello @pdecks I have updated the PR with your feedback.

@pdecks
Copy link
Contributor

pdecks commented Aug 15, 2022

It looks like the linter test started failing after this merge commit: cd567a8

@asahasrabuddhe
Copy link
Contributor Author

It looks like the linter test started failing after this merge commit: cd567a8

This is now fixed @pdecks

pdecks
pdecks previously approved these changes Aug 16, 2022
@asahasrabuddhe
Copy link
Contributor Author

@pdecks When can we expect a new release with this PR?

@pdecks
Copy link
Contributor

pdecks commented Sep 14, 2022

@pdecks When can we expect a new release with this PR?

You'll need to address the error in the pre-commit check first.

@asahasrabuddhe
Copy link
Contributor Author

@pdecks When can we expect a new release with this PR?

You'll need to address the error in the pre-commit check first.

I've fixed the pre-commit check but now the pre-commit.ci is failing. I have looked at the error and it does not look to be failing due to the code. There's some connection timeout error there.

@pdecks
Copy link
Contributor

pdecks commented Sep 20, 2022

@pdecks When can we expect a new release with this PR?

You'll need to address the error in the pre-commit check first.

I've fixed the pre-commit check but now the pre-commit.ci is failing. I have looked at the error and it does not look to be failing due to the code. There's some connection timeout error there.

Thanks for the heads up -- a teammate had enabled pre-commit.ci right after I merged the previous commit, and it looks like we're running into something akin to this issue pre-commit-ci/issues#29. We'll disable it for now.

lang/go/goimports.go Outdated Show resolved Hide resolved
To re-run tests / clear pre-commit.ci failure.
@pdecks pdecks enabled auto-merge (squash) September 20, 2022 19:41
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