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

graph: repository and module clustering #60

Merged
merged 3 commits into from
Apr 2, 2022

Conversation

ChrisHines
Copy link
Contributor

I don't find the clusters chosen by goda -cluster map to concepts in Go. I think clustering based on repository and module membership provides a more useful visualization. Here's a rough draft of some changes to make goda -cluster work that way. Is this something you would consider adding to goda if it was polished up a bit more?

@egonelbre
Copy link
Member

In principle, yes. I'll take a deeper look into this PR in a bit...

The reason the "cluster" thing feels slightly awkward is that I don't have a good model how to do all the things I want to do. For example, https://pkg.go.dev/golang.org/x/tools is a single module, however probably shouldn't be represented as a single subgraph. Of course, the current hierarchical approach also doesn't fit nicely either.

Similarly, I wanted to have some way of collapsing and un-collapsing the graph... #41 -- however, that shouldn't be too much additional complexity on the top. e.g. what happens when you collapse multiple modules in to a single "item". The main reason for collapsing are things like internal/*, which don't add much useful things to the graph. This means that it would need a way to represent "module", "package" and "group" in the same graph and links between them.

And final issues is that is "versions". e.g. what if you want query a delta between different versions of packages. Even now there's a slight version drift problem, when you query go list A:all - B:all then the subpackage versions in A and B might be different.

@ChrisHines
Copy link
Contributor Author

Yes. I understand that you have more ambitious plans, and they sound nice and powerful. I didn't want to tackle all of that at once, but I did want a way to visualize repository and module boundaries and have module versions displayed on the graph as well. So that's what this set of changes does. I can use my fork for my immediate needs, but I wanted to share what I did with you in case it was useful.

It seems to me that implementing your full ambitions will require additions to the query language or maybe adding a "layout language" to control how the packages in the queried package-set map to the output. That was more than I wanted to think about while I was still learning the goda code base.

I'm happy to provide feedback or help brainstorm ideas if you want.

@egonelbre
Copy link
Member

egonelbre commented Mar 27, 2022

Ah, I definitely didn't mean that the plans should happen in this PR. As long as something is an improvement, then I'm all for it.

I'm giving more context to the problem, in case you have some nice ideas :). So yeah, those topics are more of an over-arching goal and I don't want to block things that are immediately useful. Module based grouping would've been part of the final solution as well.

Copy link
Member

@egonelbre egonelbre left a comment

Choose a reason for hiding this comment

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

Few tiny comments, but I can adjust them later as well. Otherwise looks great.

internal/pkggraph/graph.go Outdated Show resolved Hide resolved
internal/pkggraph/tree.go Outdated Show resolved Hide resolved
internal/graph/dot.go Outdated Show resolved Hide resolved
@ChrisHines ChrisHines marked this pull request as ready for review April 2, 2022 01:57
@ChrisHines
Copy link
Contributor Author

As expected moving pkggraph.Tree into its own package clarified the names and meant we could avoid renaming pkggraph.Node. I've made this ready for review. Let me know if you see anything else to improve.

@egonelbre egonelbre merged commit 46862b1 into loov:master Apr 2, 2022
@egonelbre
Copy link
Member

Looks good to me. Thanks.

@ChrisHines ChrisHines deleted the mod-clustering branch April 8, 2022 15:00
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.

2 participants