-
Notifications
You must be signed in to change notification settings - Fork 1k
Extract command implementions into sub-packages #609
Comments
Still new to the dep, but I could try to tackle this one. Would it be okay to PR in stages? I'd assume starting by packaging up each command would be the easiest? |
Oops! 😊 Before work can start working on this, we first need to decide what the subpackages will be, which wasn't clear at all from the issue description. |
This is great! I'd say that focusing on refactoring the command structure so that we can have discrete subcommand packages is definitely step one. Let's say this issue is for that specifically, and ignore the rest for now. Because...
Splitting up gps much more than it is right now is likely to be quite a chore, as it relies on package-internal type magic for |
FYI, my mental timeline for this is:
I suggest extracting command implementations into The The cli command handlers in When something is needed by multiple commands, such as |
init, status, and ensure are bad package names.
…On Wed, May 24, 2017 at 5:32 AM, Carolyn Van Slyck ***@***.*** > wrote:
FYI, my mental timeline for this is:
1. finish in-flight init work to avoid bad merges: #500
<#500>, #589
<#589>
2. move init into a sub-package
3. move remaining commands into sub-packages
4. finally tackle getting the ensure command to match the new spec
I suggest extracting command implementations into github.com/golang/dep/<
command-name>, e.g. github.com/golang/dep/init. In each sub-package just
the command and a single entrypoint function are exposed.
The init, ensure, and status commands would be extracted into the new
sub-packages. The remove command will not be touched, as we are working
in #481 <#481> to get rid of this
deprecated command. I don't think hash-inputs is going to make it into
the final set of commands either, so that should remain in place as well.
The cli command handlers in cmd/dep are responsible for flag parsing and
other cli concerns, everything else should be moved into the sub-package.
The handlers shouldn't be doing heavy lifting, such as simultaneously
parsing args and tweaking the solver params like ensure does currently
<https://github.com/golang/dep/blob/3f525d0f4f927fc6154e5427f5a3104bfb27dce6/cmd/dep/ensure.go#L177>.
😀
When something is needed by multiple commands, such as deduceConstraint(),
for now it should probably go under the root package and be exposed as
minimally as possible. Making things internal should be out-of-scope, as
otherwise circular dependencies will quickly cause this to balloon and
impact many more areas of dep.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#609 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAcA73HgaafuvLSpl5wE48NBtIVMdAHks5r8zQ-gaJpZM4Ngwo5>
.
|
@davecheney given that we're trying to split these up to maximize clarity for divided maintainership, any suggestions? @carolynvs this all sounds pretty good to me, though I'll note that ensure is definitely going to need to monkey with params, and will likely also need to do some variable parsing of args. |
@davecheney <https://github.com/davecheney> given that we're trying to
split these up to maximize clarity for divided maintainership, any
suggestions?
I think this may be a mistake. There is no difference, merge wise, between
a directory of files, each holding the code for a single subcommand, and n
directories of files, each holding a single file.
…On Wed, May 24, 2017 at 9:47 AM, sam boyer ***@***.***> wrote:
@davecheney <https://github.com/davecheney> given that we're trying to
split these up to maximize clarity for divided maintainership, any
suggestions?
@carolynvs <https://github.com/carolynvs> this all sounds pretty good to
me, though I'll note that ensure is definitely going to need to monkey with
params, and will likely also need to do some variable parsing of args.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#609 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAcAxqdt7aDQYQnQeaSfWHnBFiKBTTQks5r83ATgaJpZM4Ngwo5>
.
|
Ah, our concern wasn't about merges, so much as just having clear boundary lines around who's responsible for what. I suppose we can go file-by-file, at least where necessary. (Or, just don't worry about drawing such precise lines...) |
I'd just like to see the implementations for the commands be better separated. For example, extracting methods out of the Run functions and moving common functions out of the command's file into someplace where it is more clear that it's shared. Up to you Sam ultimately, on how you'd like to see things organized/managed. 😃 |
@carolynvs OK, let's hold off on subpackaging them for now, unless we feel a strong need to do so for functional (as opposed to administrative) reasons. |
As we bring on new maintainers/contributors, it will be helpful to organize the code into sub-packages. This would separate the various components of dep, for example:
init
,ensure
, andstatus
.Split up gps into components:solver
, @sdboyer I forgot the others you mentioned 😅Shared code which are used by multiple components, which of course would never, ever be namedutil
, such asfs.go
.We also need to consider our plan for what's internal/exported which is a conversation that was started under #527.
UPDATE: Narrowed scope of this issue to only address the first bullet point.
The text was updated successfully, but these errors were encountered: