Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Allow dot-prefixed packages as valid import paths #572

Merged
merged 5 commits into from
May 16, 2017

Conversation

ascandella
Copy link
Contributor

@ascandella ascandella commented May 12, 2017

Fixes #564

//
// Note that there are some pathological edge cases this doesn't cover,
// such as a user using Git for version control, but having a package
// named "svn" in a directory named ".svn".
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we want to try and address this -- should we only skip these dirs if we're at the root of the project? Will that cause problems for submodules?

Copy link
Member

Choose a reason for hiding this comment

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

I think we're OK ignoring it for now, though I appreciate you pointing it out. It's a very, very edge case, and trying to deal with it here would do some level-busting: a fix would equate "root of repo" with "root of project". That's something that's true 99% of the time, but isn't actually entailed by the design of the system - at least, not this far down.

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

Just a naming nit, otherwise LGTM

@@ -28,6 +28,15 @@ type Package struct {
TestImports []string // Imports from all go test files (in go/build parlance: both TestImports and XTestImports)
}

// svnRoots is a set of directories we should not descend into in ListPackages when
// searching for Go packages
var svnRoots = map[string]struct{}{
Copy link
Member

Choose a reason for hiding this comment

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

vcsRoots instead of svnRoots, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol not sure where i get svnRoots from, good catch

@@ -1304,6 +1292,18 @@ func TestListPackages(t *testing.T) {
Imports: []string{"sort"},
},
},
"dotgodir/.m1p": {
Copy link
Member

Choose a reason for hiding this comment

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

I'm glad you caught these to move them, I entirely forgot they were there 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NP :)

@ascandella
Copy link
Contributor Author

Hygiene question: I did a rebase rather than a merge of master before pushing my branch. Is there guidance on this project around squashing/rebasing/merging?

@sdboyer
Copy link
Member

sdboyer commented May 16, 2017

LGTM. In we go 😄

Is there guidance on this project around squashing/rebasing/merging?

We haven't really established any formally. Informally, here's what I operate by:

  1. We just do straight merges into master, at least for now. No squashing.
  2. If you feel your branch history to be just too unsightly, then squashing out some commits is appreciated.
  3. Rebasing is the preferred method to catch up against mainline when that's necessary.
  4. If mainline has changed significantly enough that rebasing is arduous, whereas merging is easier (because the merge algorithms are smarter), then use a merge instead - but write a commit message to explain the conflicts necessitating the merge.

@sdboyer sdboyer merged commit 06a4f0b into golang:master May 16, 2017
@ascandella ascandella deleted the allow-dots branch May 16, 2017 18:01
@ascandella
Copy link
Contributor Author

Cool. I'll probably start self-squashing a la gerrit / phab / other tools I'm used to to keep lineage clean, and continue rebasing. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants