This repository has been archived by the owner on Sep 9, 2020. It is now read-only.
Honor per-project prune options correctly #1570
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This new type defers computation of the actual prune value for a given project until the method is called, rather than trying to precompute it. By deferring computation, we retain full fidelity on the original cascading inputs. Still a WIP hack - need to clean up, and make everything actually use this.
sdboyer
changed the title
sdboyer
force-pushed
the
fix-prune-opts
branch
from
cbd4214
to
53bd31c
Compare
All old types are now gone, tests are fixed, and test coverage should be adequate for emergency purposes.
sdboyer
force-pushed
the
fix-prune-opts
branch
from
53bd31c
to
877b793
Compare
ibrasho
reviewed
Jan 24, 2018
manifest.go
Outdated
| const ( | ||
| pvnone uint8 = 0 // No per-project prune value was set in Gopkg.toml. | ||
| pvtrue uint8 = 1 // Per-project prune value was explicitly set to true. | ||
| pvfalse uint8 = 2 // Per-project prune value was explicitly set to true. |
ibrasho
reviewed
Jan 24, 2018
| return ops | ||
| } | ||
|
|
||
| func defaultCascadingPruneOptions() CascadingPruneOptions { |
There was a problem hiding this comment.
Maybe export this instead of having multiple copies of it?
There was a problem hiding this comment.
i intentionally didn't export it, as i don't think it's worth polluting the package interface with it.
jmank88
reviewed
Jan 24, 2018
manifest.go
Outdated
| m, err := fromRawManifest(raw) | ||
| //warns = append(warns, checkRedundantPruneOptions(m)...) |
| @@ -91,12 +84,22 @@ const ( | |||
| pruneOptionNonGo = "non-go" | |||
| ) | |||
|
|
|||
| // Constants to represents per-project prune uint8 values. | |||
There was a problem hiding this comment.
s/represents/represent, or maybe just: Per-project prune values.
| @@ -91,12 +84,22 @@ const ( | |||
| pruneOptionNonGo = "non-go" | |||
| ) | |||
|
|
|||
| // Constants to represents per-project prune uint8 values. | |||
| const ( | |||
| pvnone uint8 = 0 // No per-project prune value was set in Gopkg.toml. | |||
There was a problem hiding this comment.
Did you consider using iota?
There was a problem hiding this comment.
yes, i preferred this form because:
- it is visually explicit, and the sole purpose of having these types is for the reader
- while it happens to be true that iota enumerates the first three values traditionally used in a trinary, iotas are not for expressing trinaries.
manifest.go
Outdated
| func fromRawPruneOptions(raw rawPruneOptions) (gps.CascadingPruneOptions, error) { | ||
| opts := gps.CascadingPruneOptions{ | ||
| DefaultOptions: gps.PruneNestedVendorDirs, | ||
| PerProjectOptions: make(map[gps.ProjectRoot]gps.PruneOptionSet), |
There was a problem hiding this comment.
Could make w/cap: len(raw.Projects)
Our TOML lib doesn't support decoding directly into interface{} types,
so our only short-term choice is to decode...for a third time, and
operate directly on that.
This will make the transition easier for people with automation already built around the old dep prune. We'll remove the logic again for the next release.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this do / why do we need it?
We don't actually honor per-project prune options correctly.
This new type defers computation of the actual prune value for a given project until the method is called, rather than trying to precompute it. By deferring computation, we retain full fidelity on the original cascading inputs.
This is an emergency fix, so i'm pushing it through fast.
fixes #1561