Feat: Display lineage / hierarchy for Proposals #4871
Conversation
There was a problem hiding this comment.
Thanks for implementing this nice feature - great work! 🙏
You're definitely heading in the right direction. My main feedback at this stage is to approach this using the "spike and stabilize" method: start by getting a simple prototype working (the spike), and once that’s in place, we can refine it (stabilize) by adding the necessary checks, patterns, and types to make it production-ready.
In general, I find that good abstractions and patterns emerge naturally from working code rather than being designed upfront.
This approach should help us keep sight of the bigger picture as we iterate. I'm excited to see the next iteration of this!
| @@ -19,31 +19,40 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| branchDesc = "Display the local branch hierarchy and types" | |||
| branchDesc = ` | |||
| Display hierarchy either for local branches or an existing proposal of stack. | |||
There was a problem hiding this comment.
| Display hierarchy either for local branches or an existing proposal of stack. | |
| Display hierarchy either for local branches or an existing stack of proposals. |
There was a problem hiding this comment.
This pull request makes two large changes:
- add
--lineagetogit town proposeto update the displayed lineage in the proposal - add
--lineagetogit town branchto display the branch lineage based on pull request information in the CLI
I suggest you implement each change in a separate pull request. For this one, let's focus on (1). Change (2) is a good idea but requires additional brainstorming, which we best do in an issue.
| readFlag := func(cmd *cobra.Command) (configdomain.ProposalLineage, error) { | ||
| 8000 | value, err := cmd.Flags().GetBool(proposalLineageLong) | |
| if err == nil && value { | ||
| return enabledAction, nil |
There was a problem hiding this comment.
Should it return true here, given that the user provided the CLI flag? CLI flags take precedence over any other form of configuration.
| ProposalLineageNone ProposalLineage = "none" | ||
| ProposalLineageComment ProposalLineage = "comment" | ||
| ProposalLineageTerminalDisplay ProposalLineage = "terminal-display" |
There was a problem hiding this comment.
The goal of all the custom types in this package is to make invalid states unrepresentable in the codebase. This eliminates entire categories of bugs.
With this in mind, this enum allows invalid states:
ProposalLineageTerminalDisplayisn't a valid value forgit town propose --lineage.ProposalLineageCommentisn't a valid value forgit town branch --lineage.
I suggest you split this custom type up into two separate custom types, where each wraps bool.
| ProposalCommentSortByCreatedAt ProposalCommentSortBy = "created_at" | ||
| ProposalCommentSortByUpdatedAt ProposalCommentSortBy = "updated_at" |
There was a problem hiding this comment.
I'm not sure we need a custom type here. We're not passing this value around extensively, it's only used to provide the "created_at" field to the GitHub connector. We don't use "updated_at" at all.
Our goal here isn't building a general-purpose GitHub connector; we're using the existing one to load proposal comments. I suggest keeping things simple for now: focus on getting a minimal version of the idea working by hard-coding things like "created_at" and "limit" for now.
Once the prototype is in place, we can review the code and look for patterns that want to emerge from the code base, like custom types, structs, or helper functions.
Does that sound reasonable?
| @@ -165,6 +186,63 @@ func (self Connector) squashMergeProposal(number int, message gitdomain.CommitMe | |||
| return err | |||
| } | |||
|
|
|||
| func (self Connector) getProposalComments(number int, configurations ...forgedomain.ConfigureProposalCommentQueryOptions) (Option[[]gitdomain.Comment], error) { | |||
There was a problem hiding this comment.
Since we only use two arguments for now (limit and sort-by), and actually always use the same values for each, I'd be okay keeping it simple and hard-coding them for now.
| func (self Connector) getProposalComments(number int, configurations ...forgedomain.ConfigureProposalCommentQueryOptions) (Option[[]gitdomain.Comment], error) { | |
| func (self Connector) getProposalComments(proposalID int) (Option[[]gitdomain.Comment], error) { | |
| // use "created_at" and 1 for sorting and limit | |
| if number <= 0 { | ||
| return None[[]gitdomain.Comment](), errors.New(messages.ProposalNoNumberGiven) | ||
| } |
There was a problem hiding this comment.
I don't think this check is necessary, since we always use proposal numbers returned by the GitHub API. If we do want to enforce that proposal numbers are > 0, a better approach might be to introduce a custom forgedomain.ProposalNumber type with a constructor that validates this constraint.
That way we avoid repeating this check in multiple places. It also helps surface errors closer to the source if an invalid proposal number is ever constructed.
| @@ -0,0 +1,7 @@ | |||
| package gitdomain | |||
|
|
|||
| type CommentBody string | |||
There was a problem hiding this comment.
Not sure this custom type is needed yet. When in doubt, leave it out 🙂
|
You can run |
No description provided.