feat: Compose groups via include-groups for reusable tabs and summaries#332
Open
vkryukov wants to merge 1 commit intoachannarasappa:masterfrom
Open
feat: Compose groups via include-groups for reusable tabs and summaries#332vkryukov wants to merge 1 commit intoachannarasappa:masterfrom
vkryukov wants to merge 1 commit intoachannarasappa:masterfrom
Conversation
- Support include-groups in ConfigAssetGroup to compose groups without duplication - Validate duplicate group names; detect and error on unknown or cyclic includes - Build effective groups by expanding includes, deduping watchlist (first-seen), concatenating holdings - Refactor getGroups: extract resolveGroupIncludes, dedupePreserveOrder, symbolsBySource helpers for clarity - Update README with examples, rules, and forward reference note - Add CLI tests for composition, unknown includes, cycles, and forward-referenced groups
Owner
achannarasappa
left a comment
achannarasappa
left a comment
There was a problem hiding this comment.
Thank you for this PR! This looks like it could be really useful
Some overall comments below:
-
I have a few smaller stylistics comments which I've added inline
-
I found that adding duplicates of the same group also included a duplicate of that group which isn't a problem for watchlist but does duplicate holdings. Example config snippet to duplicate:
- name: stocks-1
include-groups:
- stocks-2
- stocks-2
- stocks-2
holdings:
- symbol: "LMND"
quantity: 1.0
unit_cost: 100
- name: stocks-2
holdings:
- symbol: "LMND"
quantity: 1.0
unit_cost: 100
- Is user sort order preserved and deterministic when including groups?
| Name string `yaml:"name"` | ||
| Watchlist []string `yaml:"watchlist"` | ||
| Holdings []Lot `yaml:"holdings"` | ||
| IncludeGroups []string `yaml:"include-groups"` |
Owner
There was a problem hiding this comment.
I suggest changing the name to just includes with the group aspect being implied
| * If top level `watchlist` or `lots` properties are defined in the configuration file, the entries there will be added to a group named `default` which will always be shown first | ||
| * Ordering is defined by order in the configuration file | ||
| * The `holdings` property replaces `lots` under `groups` but serves the same purpose | ||
| * New: `include-groups` allows composing a group from other groups without duplicating entries. The effective watchlist is the concatenation of included groups’ watchlists (then the group’s own), de-duplicated by first occurrence; holdings are concatenated (lots aggregate by symbol automatically in summaries) |
Owner
There was a problem hiding this comment.
Not needed to call out this is new and it may not be new as time goes on.
| * The `holdings` property replaces `lots` under `groups` but serves the same purpose | ||
| * New: `include-groups` allows composing a group from other groups without duplicating entries. The effective watchlist is the concatenation of included groups’ watchlists (then the group’s own), de-duplicated by first occurrence; holdings are concatenated (lots aggregate by symbol automatically in summaries) | ||
|
|
||
| Example composite group: |
Owner
There was a problem hiding this comment.
It could be nice to include this in a collapsible secion since this likely may not be used by the majority of users:
| // Build final groups in declaration order using flattened config | ||
| for _, configAssetGroup := range configAssetGroups { | ||
| // compute effective watchlist/holdings | ||
| effWatchlist := configAssetGroup.Watchlist |
Owner
There was a problem hiding this comment.
Style nit: full word names
| // Index groups by name for include resolution and validate duplicate names | ||
| groupsByName := make(map[string]c.ConfigAssetGroup) | ||
| for _, g := range configAssetGroups { | ||
| if g.Name == "" { |
Owner
There was a problem hiding this comment.
Style nit: full word names
| // resolveGroupIncludes flattens include-groups recursively preserving order and detecting cycles | ||
| func resolveGroupIncludes(groupsByName map[string]c.ConfigAssetGroup, cur c.ConfigAssetGroup, visiting map[string]bool) ([]string, []c.Lot, error) { | ||
|
|
||
| wl := make([]string, 0) |
Owner
There was a problem hiding this comment.
Style nit: full word names
Author
|
Thanks a lot for the feedback! I hope to be able to implement your suggestions soon. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First off, thank you for creating ticker — I use it daily and it’s been incredibly useful. Also, apologies for the unsolicited PR; I hope this enhancement aligns with the project’s direction.
Summary
Why
I often want a “summary” group that combines a few existing groups (e.g., crypto + personal stocks) without duplicating config. This adds that capability while remaining backward compatible and predictable.
Config Usage
Implementation
IncludeGroups []stringtoConfigAssetGroup.resolveGroupIncludes,dedupePreserveOrder,symbolsBySource.Notes
Thank you again for ticker and for considering this improvement. Happy to adjust naming, error messages, or structure to better fit the project’s style.