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

no arguments should mean . (again) #44

Open
dmitshur opened this issue Sep 8, 2019 · 3 comments · May be fixed by #45

Comments

@dmitshur
Copy link
Contributor

commented Sep 8, 2019

This is a bug that was reported previously as #8, and was fixed in #9, but regressed in #32.

unconvert without arguments should be the same as unconvert ., that is operate on the Go package in the current working directory.

This is the cause:

unconvert/unconvert.go

Lines 221 to 224 in 2f5dc33

importPaths := flag.Args()
if len(importPaths) == 0 {
return
}

Before #32 was applied, that code looked like this:

unconvert/unconvert.go

Lines 200 to 203 in dc69f12

importPaths := gotool.ImportPaths(flag.Args())
if len(importPaths) == 0 {
return
}

The difference is that gotool.ImportPaths([]string(nil)) returned []string{"."}. This isn't the case after #32 replaced that with just flag.Args().

Note that packages.Load(cfg) returns the same result as packages.Load(cfg, "."), it loads the package in the current working directory. The current code just never reaches the call to packages.Load.

/cc @egonelbre @mvdan

P.S. I noticed this because of https://github.com/shurcooL/octicon/blob/9ff1a4cf27f4a918862c3ffbd24371b3aed18f9d/doc.go#L10.

@mvdan

This comment has been minimized.

Copy link

commented Sep 8, 2019

If this is fixed once again, consider adding a test via https://godoc.org/github.com/rogpeppe/go-internal/testscript. Otherwise one of these edge cases is bound to be broken in the future again :)

@dmitshur

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

@mdempsky

This comment has been minimized.

Copy link
Owner

commented Sep 8, 2019

PRs to add proper testing are welcome. :)

@egonelbre egonelbre referenced a pull request that will close this issue Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.