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

Hide dep guts under internal/ #527

Closed
wants to merge 4 commits into from
Closed

Hide dep guts under internal/ #527

wants to merge 4 commits into from

Conversation

carolynvs
Copy link
Collaborator

This is an experiment to see if we like having dep's guts moved into internal.

I was trying to implement #500 under internal and was running into circular dependency hell. If we don't go with something like this, my plan is to implement #500 outside of internal, all in one directory to stop wasting time with circular reference problems. 😀

I realize this touches pretty much ... everything so here's the tldr:

  • Move dep/ to internal/dep/.
  • Move testdata/ to internal/dep/testdata.
  • Move log functions into own internal package so test, internal pkgs and cmd/dep can all reference.
    Otherwise circular references were present.
    Using the name util in honor of @kris-nova and to ensure proper bike-shedding.
  • Move test/ into internal/test/ so that internal tests can use it.

The following packages were left exported:

  • gps because it's crazy useful and doesn't have circular reference problems that would make me want to move it.
  • hack because anyone who references that is silly anyway.
  • cmd/dep because a) I'd like to see everything interesting in that moved into internal anyway, there's a lot of logic in there and b) again it isn't having circular reference problems.

This is the final folder structure (though the commits are broken down if we want to not do all of this):

  • dep/
    • cmd/
    • gps/
    • hack/
    • internal/
      • cfg/
        • manifest.go
        • lock.go
      • dep/
        • context.go
        • project.go
        • ... (rest of the stuff from the root)
      • test/
      • util/
        • fs.go
        • log.go
    • ... non-go stuff from the root is still here

@carolynvs
Hide dep guts under internal/
15f30d9 
* Move dep/ to internal/
* Move testdata/ to internal/testdata
* Move log funcs into own internal package so test, internal and cmd/dep can reference.
  Otherwise circular references were present.
  Using the name util in honor of @kris-nova and to ensure proper bike-shedding.
* Move test/ into internal/test/ so that internal tests can use it
@carolynvs
Move manifest and lock into internal/cfg
8aba147 
This lets other packages reference the configuration files
@carolynvs
Move filesystem functions into util
853e602 
There’s an awkward cycle between util and test. One way to resolve would be to bring test into util.
For now, I kept fs_test in internal instead of moving it into util too.
@carolynvs
Don’t have anything in internal directly
3d610dc 
Moving internal/* to internal/dep/*. This keeps all our old references the same as before
e.g. dep.Ctx stays the same instead of internal.Ctx
@dcheney-atlassian
Copy link

LGTM with the exception of internal/util which is a dumping ground of unrelated functionality.

@carolynvs
Copy link
Collaborator Author

@dcheney-atlassian Yeah that's why I left the comment invoking @kris-nova because she just started a conversation about this topic. I didn't want to spend too much time time coming up with pkg names and knew that calling it util would bring people out to share their opinions. 😀

@jmank88
Copy link
Collaborator

jmank88 commented May 8, 2017 •
edited
Loading

If/when this converges with my logger changes or some version of them (#525), then log.go goes away, and that will be a few less util members to worry about.

@krisnova
Copy link
Contributor

krisnova commented May 8, 2017

Thanks for the love @carolynvs,

Twitter shenanigans aside, what are we gaining from the move? Unsure what the inspiration for this awesome change is/was?

Also re:

I was trying to implement #500 under internal and was running into circular dependency hell

Which packages were conflicting? There are a few easy ways to pull them apart if necessary.. but usually when you get cycles it's because 1) you have too many packages or 2) an interface is defined somewhere weird or something. Just curious is all :)

@carolynvs
Copy link
Collaborator Author

@kris-nova This was just a thought experiment (heavily aided by an IDE, so it wasn't a huge time investment). 😁

There has been encouragement to avoid exporting new types/functions, and when I was chatting with @sdboyer yesterday, I asked What would dep look like if we made everything internal? Instead of trying hard to not export things, and end up with a single flat directory, what if we moved most of dep into internal/ so that we could organize our code without worrying about exports. I volunteered to try it out and if it wasn't a steaming pile of 💩, open a PR so people could discuss it.

For #500 in particular, when I tried to implement it under internal/, I was running into trouble because I needed things like the manifest, lock and context, which are defined under dep, which also reference internal/. I also wanted to group some of my files into a directory (gasp I know what was I thinking?), but quickly ran into trouble because of where the log functions are defined. Instead of trying to move the things I needed in this PR, I wanted to step back and think as a group about how we'd like to organize the code in dep.

I was hoping we could look at this and answer a couple questions:

  • This is what (nearly) everything in interna/ looks like. Do we like it?
  • If we try to not export anything, does that mean that everything ends up in a single flat directory, most likely in cmd/dep since that's at the top of the import graph?
  • Do we want to hide only new functions/types or do we want existing stuff to be unexported too?

@carolynvs
Copy link
Collaborator Author

@jmank88 Sweet! I didn't realize your in-proc changes would improve our logging situation too! 👍

@sdboyer
Copy link
Member

sdboyer commented May 9, 2017

Some quick thoughts on this...

I do like the idea of moving almost everything into internal, at least initially, because it's easier to expose that stuff later when we're confident than it is to play footsie with general community best practices about releasing Go software in a project that's poised to influence those practices. That's not an ouroboros I want.

It seems like there's three basically sensible things we could do here:

  1. Expose...nothing. At least for now.
  2. Expose gps, dep.Manifest, dep.Lock, and possibly also dep.Ctx.
  3. Hide away e.g. the test package, maybe also the glide/other tool-related packages, but leave the rest exposed.

3 is mostly a 🦆.

1 seems too draconian, but 2 seems a bit too open, and still problematic because we're absolutely still modifying those types. ...and actually, 2 seems remarkably similar to what we're already doing right now 😦.

All that's to say, I'm open to argument here, and would ❤️ it if someone made a clear, strong one 😄

also, just

gps because it's crazy useful

😂 😂 😂 ❤️

@davecheney
Copy link
Contributor

davecheney commented May 9, 2017 via email

@carolynvs
Copy link
Collaborator Author

While I realize option 1 feels draconian, anyone who would be tempted to rely on this code right now is going to be burned. I just checked godoc and it doesn't appear that anyone is referencing dep yet. One could take that to mean that it's not worth bothering, or as the perfect time to move code before people do start referencing it.

I don't have skin in the game, we could go either way exposing/hiding everything and I'd be fine. What is chaffing me right now is that concerns about exposing new code is making it harder for me to work on features. I am spending too much time thinking about where code should be. 😊 Maybe I'm the only one feeling that way, I am new-ish to Go. But I would love for us to decide and commit to one or other.

@davecheney
Copy link
Contributor

davecheney commented May 9, 2017 via email

@sdboyer
Copy link
Member

sdboyer commented May 10, 2017

@davecheney this is a significant and general enough issue that i'm going to explicitly pull in other committee folks, so the decisionmaker in this case will be that group (though i may still be the one who clicks 'merge').

that's just decisionmaking, though. i'm hoping someone will step up with a coherent proposal for what of the codebase to make internal vs. not, as well as at least some general outline of how we should approach the question of publicizing packages in the future.

i, at least, do not plan to make any such proposal (so, please ignore my earlier comment), though i'll certainly participate in discussing those that are made.

@davecheney
Copy link
Contributor

davecheney commented May 10, 2017 via email

@sdboyer
Copy link
Member

sdboyer commented May 10, 2017

(I'll assume #521 is also included alongside #519)

Additional questions to answer would be:

  • What do we do with the github.com/golang/dep package?
    • If it remains exposed, what guideline do we follow about identifiers that are OK to export?
  • At what time in the future would it be appropriate to consider re-exposing some of these packages (e.g. gps or github.com/golang/dep)

@adg
Copy link
Contributor

adg commented May 10, 2017

I'm in favor of moving everything into internal/ (including gps) until we have concrete plans about what we want to make available.

@dcheney-atlassian
Copy link

dcheney-atlassian commented May 10, 2017 via email

@sdboyer
Copy link
Member

sdboyer commented May 10, 2017

Nope, scope creep is what is lead to this issue bogging down. #519
addresses just github.com/golang/dep/gps

If this discussion were on #519, it'd be scope creep. But the scope of this issue has, from the outset, been inclusive of moving (potentially) all packages under internal/.

I opted to turn toward policy as we're organically bumping into this elsewhere, and it's clear that the lack of even a loose policy on this is hindering development.

@davecheney
Copy link
Contributor

davecheney commented May 10, 2017 via email

@sdboyer
Copy link
Member

sdboyer commented May 10, 2017

no problem. seems pretty clear that any outcome here is going to end up putting gps under internal for the moment, so i'd be happy to merge a PR that did so; perhaps it'll help narrow the discussion here in pursuit of a policy.

@spenczar spenczar mentioned this pull request May 13, 2017
Closed
@carolynvs carolynvs mentioned this pull request May 19, 2017
Closed
@carolynvs
Copy link
Collaborator Author

I'd still like to see us come to an understanding of what contributors should be aiming for with respect to what's exposed. Maybe that can be considered in #609. 😄

@carolynvs carolynvs closed this May 22, 2017
@carolynvs carolynvs deleted the hide-all-the-things branch May 22, 2017 16:12
@carolynvs carolynvs mentioned this pull request May 29, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@carolynvs @dcheney-atlassian @jmank88 @krisnova @sdboyer @davecheney @adg @googlebot