Uh oh!
There was an error while loading. Please reload this page.
refactor: extract shared project config parsing helpers to eliminate duplication - #20939
Merged
pelikhan merged 5 commits intoMar 14, 2026
Merged
Conversation
…duplication Extract parseProjectViews and parseProjectFieldDefinitions from the duplicated blocks in parseCreateProjectsConfig and parseUpdateProjectConfig into a shared pkg/workflow/project_config_parsing.go file. Both methods now delegate to the helpers, eliminating ~100 lines of duplicated code and ensuring a single source of truth for views/field-definitions parsing. Also adds parity tests in project_config_parsing_test.go covering the helpers directly and verifying identical behavior from both the create and update paths. Fixes #<issue>" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
5 tasks
Copilotcreated this pull request from a session on behalf of pelikhanMarch 14, 2026 14:15 View session
pelikhan
marked this pull request as ready for review
March 14, 2026 14:21
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors duplicated views and field-definitions parsing logic out of the create/update project config paths into shared helpers to reduce drift and centralize behavior.
Changes:
- Added shared parsing helpers for project
viewsandfield-definitions. - Updated create-project and update-project config parsing to call the shared helpers.
- Added new unit tests covering the helpers plus a parity check across both command paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/workflow/project_config_parsing.go | New shared parsing helpers for views and field-definitions. |
| pkg/workflow/create_project.go | Replaces inline parsing with calls to shared helpers. |
| pkg/workflow/update_project.go | Replaces inline parsing with calls to shared helpers. |
| pkg/workflow/project_config_parsing_test.go | New table-driven tests for helper behavior and create/update parity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+18
to
+136
| func TestParseProjectViews(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input map[string]any | ||
| expected []ProjectView | ||
| }{ | ||
| { | ||
| name: "no views key", | ||
| input: map[string]any{}, | ||
| expected: nil, | ||
| }, | ||
| { | ||
| name: "empty views list", | ||
| input: map[string]any{ | ||
| "views": []any{}, | ||
| }, | ||
| expected: nil, | ||
| }, | ||
| { | ||
| name: "single valid view with required fields only", | ||
| input: map[string]any{ | ||
| "views": []any{ | ||
| map[string]any{ | ||
| "name": "Board", | ||
| "layout": "board", | ||
| }, | ||
| }, | ||
| }, | ||
| expected: []ProjectView{ | ||
| {Name: "Board", Layout: "board"}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "view with all optional fields", | ||
| input: map[string]any{ | ||
| "views": []any{ | ||
| map[string]any{ | ||
| "name": "Task Board", | ||
| "layout": "board", | ||
| "filter": "is:issue", | ||
| "visible-fields": []any{1, 2, 3}, | ||
| "description": "Main board", | ||
| }, | ||
| }, | ||
| }, | ||
| expected: []ProjectView{ | ||
| { | ||
| Name: "Task Board", | ||
| Layout: "board", | ||
| Filter: "is:issue", | ||
| VisibleFields: []int{1, 2, 3}, | ||
| Description: "Main board", | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "view missing name is skipped", | ||
| input: map[string]any{ | ||
| "views": []any{ | ||
| map[string]any{"layout": "board"}, | ||
| }, | ||
| }, | ||
| expected: nil, | ||
| }, | ||
| { | ||
| name: "view missing layout is skipped", | ||
| input: map[string]any{ | ||
| "views": []any{ | ||
| map[string]any{"name": "Board"}, | ||
| }, | ||
| }, | ||
| expected: nil, | ||
| }, | ||
| { | ||
| name: "mix of valid and invalid views", | ||
| input: map[string]any{ | ||
| "views": []any{ | ||
| map[string]any{ | ||
| "name": "Valid", | ||
| "layout": "table", | ||
| }, | ||
| map[string]any{ | ||
| // Missing layout | ||
| "name": "Invalid", | ||
| }, | ||
| map[string]any{ | ||
| "name": "Also Valid", | ||
| "layout": "roadmap", | ||
| }, | ||
| }, | ||
| }, | ||
| expected: []ProjectView{ | ||
| {Name: "Valid", Layout: "table"}, | ||
| {Name: "Also Valid", Layout: "roadmap"}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "non-map view item is skipped", | ||
| input: map[string]any{ | ||
| "views": []any{ | ||
| "not-a-map", | ||
| map[string]any{ | ||
| "name": "Good", | ||
| "layout": "board", | ||
| }, | ||
| }, | ||
| }, | ||
| expected: []ProjectView{ | ||
| {Name: "Good", Layout: "board"}, | ||
| }, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := parseProjectViews(tt.input, testLog) | ||
| assert.Equal(t, tt.expected, result, "parseProjectViews result should match expected") | ||
| }) | ||
| } |
Comment on lines
+24
to
+35
| { | ||
| name: "no views key", | ||
| input: map[string]any{}, | ||
| expected: nil, | ||
| }, | ||
| { | ||
| name: "empty views list", | ||
| input: map[string]any{ | ||
| "views": []any{}, | ||
| }, | ||
| expected: nil, | ||
| }, |
pelikhan
commented
Mar 14, 2026
Collaborator
@copilot review comments |
- Add wrong container type cases for views and field-definitions (string/map instead of []any) to lock in nil-early-return behavior - Add mixed-type visible-fields and options cases to document that non-int/non-string elements are silently skipped - Use assert.Empty for nil-expected cases to avoid brittle nil-vs-empty-slice comparisons Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
ContributorAuthor
Addressed both review comments in c398e7c:
|
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
parseCreateProjectsConfigandparseUpdateProjectConfigcontained ~100 lines of identicalviewsandfield-definitionsparsing logic, creating a divergence risk where fixes or schema changes had to be applied twice.Changes
pkg/workflow/project_config_parsing.go(new): two standalone helpers:parseProjectViews(configMap map[string]any, log *logger.Logger) []ProjectViewparseProjectFieldDefinitions(configMap map[string]any, log *logger.Logger) []ProjectFieldDefinitioncreate_project.go/update_project.go: inline parsing blocks replaced with calls to the shared helpers; per-file loggers are passed through so debug output remains contextual.pkg/workflow/project_config_parsing_test.go(new): table-driven tests for each helper (missing keys, invalid items, all optional fields) plus a parity test asserting both command paths produce identical results for the same input.