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

Support AllowUnexportedWithinModule #116

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support AllowUnexportedWithinModule #116

wants to merge 1 commit into from

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Feb 25, 2019

For sufficiently large codebases with many struct types defined,
it is untenable to specify a large list of all types in the repository
that AllowUnexported permits visibility for.

In Go, we observe that code with a shared owner often lives in the same
repository, and all sub-packages within a repository often share a common
package path prefix. Thus, permit expressing visibility allowances in terms
of package path prefixes instead of individual types.

While not explicitly documented, this feature allows the universal comparing
of unexported fields on all types:
cmp.Equal(x, y, cmp.AllowUnexportedWithinModule(""))

This "feature" is intentionally undocumented (but a natural side-effect of the
expressed behavior of PackagePrefix) since it considered to be bad practice.

@dsnet
Copy link
Collaborator Author

dsnet commented Feb 25, 2019

A problem with PackagePrefix is that it assumes that package paths are universally stable. There are two failure cases I can think of:

  • A type is moved out of a repo such that the package it was declared in changed. However, most movement of types is most likely to be within the same repo, so still shares a common package prefix. However, the movement of a type due to type aliases implies that the test author moved the type and can update the test accordingly.
  • In non-open-source code-bases, vendoring of third-party packages sometimes re-writes the import paths. However, this seems to me to be more a problem with the vendoring technology.

Copy link
Collaborator

@neild neild left a comment

Choose a reason for hiding this comment

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

Test?

cmp/options.go Outdated Show resolved Hide resolved
cmp/options.go Outdated Show resolved Hide resolved
@dsnet dsnet force-pushed the export-prefix branch 6 times, most recently from 979f9b0 to 6c80e02 Compare February 26, 2019 00:22
@neild
Copy link
Collaborator

neild commented Feb 26, 2019

LGTM. (Is there no button to press for LGTM? If so, I can't find it. I miss Gerrit.)

For sufficiently large codebases with many struct types defined,
it is untenable to specify a large list of all types in the repository
that AllowUnexported permits visibility for.

In Go, we observe that code with a shared owner often lives in the same
repository, and all sub-packages within a repository often share a common
package path prefix. Thus, permit expressing visibility allowances in terms
of package path prefixes instead of individual types.

While not explicitly documented, this feature allows the universal comparing
of unexported fields on all types:
	cmp.Equal(x, y, cmp.AllowUnexportedWithinModule(""))

This "feature" is intentionally undocumented (but a natural side-effect of the
expressed behavior of PackagePrefix) since it considered to be bad practice.
@dsnet
Copy link
Collaborator Author

dsnet commented Feb 26, 2019

Apologies for the flurry of changes; was struggling to find a good name. I settled on AllowUnexportedWithinModule. A module is defined as "a collection of related Go packages that are versioned together as a single unit", which seems to be exactly what we want here.

(Thumbs up if the name is fine with you)

@dsnet dsnet changed the title Support PackagePrefix in AllowUnexported Support AllowUnexportedWithinModule Feb 26, 2019
@neild
Copy link
Collaborator

neild commented Feb 26, 2019

Hm. "Module" has a very specific definition, however, revolving around go.mod files. I think we should stay away from it unless we really mean it.

"PackagePrefix" is verbose, but seems pretty clear.

@warpfork
Copy link

I will be fond of this feature when it lands. :)

+1 towards "PackagePrefix" for naming. I saw the word "Module" in the commit message and had a brief frission of shock and confusion (e.g. how and why would a library like this know about and read 'go.mod' files?) until reading the full message and patch.

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

3 participants