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

Automatically detect local prefixes #2363

Closed

Conversation

sagikazarmark
Copy link

This PR is an attempt to automatically detect local prefixes from module paths.
The rationale behind this feature is making config files less verbose/redundant and a little bit more portable.

It is not perfect, but it's working.

More specifically, I'm not sure if loading module information for every linter is a good idea, so module information loading should be incorporated into the LoadMode, but I'm not sure how since it's a string.

I'm also not sure if the gci -> goimports fallback is necessary.

Last, but not least the config option name feels a bit weird, but I couldn't come up with a better one.

Any feedback would be highly appreciated.

Fixes #931

@boring-cyborg
Copy link

boring-cyborg bot commented Nov 17, 2021

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Nov 17, 2021

CLA assistant check
All committers have signed the CLA.

@ldez ldez self-requested a review November 17, 2021 01:01
@ldez ldez added the enhancement New feature or improvement label Nov 17, 2021
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.

it is better to grab the local prefix from go.mod file if it exists during the initialization of linter itself
and please add the tests

@sagikazarmark
Copy link
Author

@SVilgelm thanks for the feedback!

Is there already an example somewhere that reads the go.mod file or uses the module name? Speaking from experience, it can get tricky.

@SVilgelm
Copy link
Member

Here is an example of parsing go.mod file

import (
	"golang.org/x/mod/modfile"
)

func getModulePath() string {
	mfile, _ := modfile.ParseLax("go.mod", []byte{}, nil)
	return mfile.Module.Mod.Path
}

@ernado
Copy link
Member

ernado commented Nov 17, 2021

Great idea in general, just please make sure to make it opt-in so we don't break backward compatibility (looks like it is opt in via local-prefix-module: true which is good)

Comment on lines +36 to +61

if localFlag == "" && lintCtx.Settings().Gci.LocalPrefixModule {
var modules []string

for _, module := range lintCtx.Modules {
modules = append(modules, module.Path)
}

localFlag = strings.Join(modules, ",")
}

goimportsFlag := lintCtx.Settings().Goimports.LocalPrefixes
if localFlag == "" && goimportsFlag != "" {
localFlag = goimportsFlag
}

if localFlag == "" && lintCtx.Settings().Goimports.LocalPrefixModule {
var modules []string

for _, module := range lintCtx.Modules {
modules = append(modules, module.Path)
}

localFlag = strings.Join(modules, ",")
}

Copy link
Member

Choose a reason for hiding this comment

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

These looks exactly the same with some duplicated code, maybe would be nice to create a function?

localFlagFn := func(modules) string {
	var modules []string

	for _, module := range lintCtx.Modules {
		modules = append(modules, module.Path)
	}

	return strings.Join(modules, ",")
}

Then just call it twice:

if localFlag == "" && lintCtx.Settings().Gci.LocalPrefixModule {
	localFlag = localFlagFn(lintCtx.Settings().Gci.LocalPrefixModule)
}

goimportsFlag := ...


if localFlag == "" && lintCtx.Settings().Goimports.LocalPrefixModule {
	localFlag = localFlagFn(lintCtx.Settings().Goimports.LocalPrefixModule)
}

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely!

@sagikazarmark
Copy link
Author

@SVilgelm I've spent some time trying to figure out how to get the most accurate module information and it's a little bit complicated than just calling that modfile package.

First, we need to find the go.mod file. The easiest (and probably the most accurate) way to do it is running go env GOMOD (that requires the Go tool to be in the path though). Then we can parse the go.mod file.

It might also make sense to just defer this task to a library that already does this well: https://github.com/gobuffalo/here

Last, but not least: I don't know if it's possible to analyse multiple packages from different modules at the same time, but if it is, then the current solution will provide the most accurate result. (It doesn't necessarily have to live in the context though, can be moved to the linter implementations as well.

What do you think?

@ldez
Copy link
Member

ldez commented Nov 21, 2021

you can take a look at what I did here: https://github.com/ldez/gomoddirectives/blob/82673faed925c794e4a179ae238dd0b10f260732/module.go#L22-L24

Personally, I think that autodetecting is not a good idea because it will not work well in some cases (like golangci-lint by file inside an IDE), and I think it's an overcomplicated solution for something that is not a real problem.

@sagikazarmark
Copy link
Author

Can we get to an agreement for whether this change would be merged or not before I invest any more time in it?

@ldez that's a fair point and I think the current solution would be better from that perspective as well (if no module info is available, no prefix will be applied, whereas manually looking for the go.mod file might be problematic).

@ldez
Copy link
Member

ldez commented Mar 29, 2024

Fixed by #4522

@ldez ldez closed this Mar 29, 2024
@ldez ldez added declined and removed enhancement New feature or improvement labels Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add prefix-current-module to goimports
6 participants