-
Notifications
You must be signed in to change notification settings - Fork 1k
Conversation
Sample output!
|
LGTM. I would like to see the return of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basic refactors look good, though haven't actually tested it locally just yet.
mostly some nits on the actual helptext.
thanks again for doing this, Peter!
|
||
Package specs: | ||
<path>[:alt location][@<version specifier>] | ||
const ensureShortHelp = `Ensure a dependency is vendored in the project` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be good to just add one word:
Ensure a dependency is safely vendored in the project
"safely" will let people know there's some work being done there beyond simply fetching a project, and is amorphously indicative of the general kind of work that's happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
Ensure is used to fetch project dependencies into the vendor folder, as well as | ||
to set version constraints for specific dependencies. It takes user input, | ||
solves the updated dependency graph of the project, writes any changes to the | ||
manifest and lock file, and downloads dependencies to the vendor folder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"downloads dependencies" is slightly misleading, as it's often/usually doing a copy of something that may have been downloaded long ago. Maybe:
places dependences in the vendor directory.
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
const ensureExamples = ` | ||
dep ensure | ||
|
||
Solve the project's dependency graph, and download all dependencies to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeat of above re: "download"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
specified there. Otherwise, use the most recent version that can satisfy the | ||
constraints in the manifest file. | ||
|
||
dep ensure -update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we haven't actually implemented this flag yet - probably should make a note of that in the helptext?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack; actually I have just deleted all references to it, I think that's easier.
Same as above, but choose any release matching 0.9.x, preferring latest. If | ||
a constraint was previously set in the manifest, this resets it. | ||
|
||
dep ensure github.com/heroku/rollrus@^0.9.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supernit: i'd like carat above tilde, as it's the form we prefer people to use, so i'd like them to read it first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely!
|
||
dep ensure github.com/heroku/rollrus | ||
|
||
Update a specific dependency to the latest version allowed by the manifest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't actually a supported form right now; even though it was fixed in the other PR, we just haven't dealt with the single-dep update case at all yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack; scratching, re: below.
dep ensure github.com/heroku/rollrus | ||
|
||
Update a specific dependency to the latest version allowed by the manifest, | ||
including all of its transitive dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think we'd ever actually be able to support this. while we can allow specifying multiple individual deps for update, we can't readily do all the transitive deps of a named dep, too. (a structural refactor in gps - sdboyer/gps#44 - may enable it, but even then, i'm not sure it's a good idea) plus, would this refer to the transitive dependencies that are uniquely the deps of the named project? or would it update shared/diamond deps, as well? this gets complicated.
for now, it might just be best to drop this whole individual example, until we sort out how we want it to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack; scratching this example altogether.
ensureCmd.flag.Var(&overrides, "override", "Interpret specified constraint(s) as override(s) rather than normal constraints") | ||
Fetch the dependency from a different location. | ||
|
||
dep ensure github.com/heroku/rollrus==1.2.3 # 1.2.3 exactly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's drop this whole example, we actually got rid of the double-operator form entirely, with the intention of figuring out a different way to express the same idea later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, dropping both double-operator examples and the text.
|
||
Note: init may use the network to solve the dependency graph. | ||
|
||
Note: init does NOT vendor dependencies. See dep ensure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may make it vendor deps sometime soon - maybe make this say
init does NOT vendor dependencies (for now).
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
Show documentation for the dep tool or the specified command. | ||
`, | ||
}) | ||
// Easy peasy livin' breezy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏄♀️🌻
- short/long should not embed leading/trailing newlines - Normalize grammar in short texts, and remove trailing periods - Remove tabs from strings that will be printed to terminal - Use tabwriter to print table of subcommands
Sorry for this big change. I think it's a big improvement. First, I restructured how commands work. - Commands are now structs that implement the command interface - Flags are bound to parameters in the command struct - Run is a method on the command struct, with access to flag vars This allowed me to make a lot of consistency improvements. - Good news: dep foo -v ... works - Bad news: dep -v foo ... stopped working (but that was weird) - Good news: dep foo -h works and looks consistent - Bad news: dep help foo doesn't exist anymore (but that was weird) With all of that in place, I was able to audit and refactor all of the help texts. I think they're a lot better. The biggest individual change was that I moved all of the ensure usage examples to a special flag, ensure -examples. I hope that's acceptable. If I screwed anything else up, please accept my apologies.
|
||
dep ensure github.com/heroku/rollrus | ||
dep ensure github.com/heroku/rollrus@^0.9.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shit, i missed this on my first pass - in the examples, we should use something > 1.0.0. The rules for the unary constraint operators change below 1.0.0, and I don't think we want people focused on that in the simplest of help text contexts.
9882c28
to
92b700e
Compare
dep ensure -override github.com/heroku/rollrus@^0.9.1 | ||
|
||
Forcefully and transitively override any constraint for this dependency. | ||
This can inadvertantly make your dependency graph unsolvable; use sparingly. | ||
This can inadvertantly make your dependency graph unsolvable; use sparingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also missed this, sorry - the problem with overrides isn't that they make the depgraph unsolvable (indeed, the opposite - they're the sledgehammer you use when you can't solve the degraph any other way). Here's an alternative:
Overrides are powerful, but they are harmful over the long term. They should be used as a last resort, especially if any packages in your project may be imported by others.
Sorry for this big change. I think it's a big improvement. First, I restructured how commands work.
This allowed me to make a lot of consistency improvements.
dep foo -v ...
works as expecteddep -v foo ...
stopped working — but that was weird anywaydep foo -h
works and looks consistentdep help foo
doesn't exist anymore — but that was weird anywayWith all of that in place, I was able to audit and refactor all of the help texts. I think they're a lot better. Please take a look at any commands you feel particularly close to, and make sure I didn't write any lies. The biggest individual change was that I moved all of the ensure usage examples to a special flag,
ensure -examples
. I hope that's acceptable.If I screwed anything else up, please accept my apologies.
Closes #73.