refactor: centralize item payload construction with simplified format handling#23
refactor: centralize item payload construction with simplified format handling#23
Conversation
…utility Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>
Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>
Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>
…get_payload Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>
| base_payload["definition"] = {"parts": definition["parts"]} | ||
| else: | ||
| # Default to ipynb format | ||
| base_payload["definition"] = { |
There was a problem hiding this comment.
@copilot enhance that section. the elif item_type in (), should check if the item type is NOT supported, and raise exception, otherwise it should do
base_payload["definition"] = definition
and return base_payload.
secondly, try to extract the format from args and if not exists try to extract from the definition_format_mapping default value, instead of hardcoding it.
There was a problem hiding this comment.
Enhanced the logic as requested:
- ✅ Changed to check if item type is NOT supported (using
unsupported_typeslist) - new supported types will work automatically without code changes - ✅ Extract format from
definition_format_mappinginstead of hardcoding - parses query param format (e.g., "?format=ipynb" → "ipynb") - ✅ All other item types (not in unsupported list) use definition directly
Commit: bb8f01e
The function now:
- Checks against an explicit unsupported types list
- Extracts format values from
definition_format_mappingfor Notebook and SparkJobDefinition - Maintains original behavior (.py format has no format key)
- All tests passing ✅
…format_mapping - Changed logic to check if item type is NOT supported (using unsupported_types list) - Extract format values from definition_format_mapping instead of hardcoding - Parse format query param strings (e.g., "?format=ipynb" -> "ipynb") - Maintain original behavior: .py format has no format key, other formats use mapping - All other item types that are not in unsupported list use definition directly Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>
| return fab_item_util.build_item_payload( | ||
| item, | ||
| definition=definition, | ||
| description="Imported from fab", |
| } | ||
| # Use centralized payload builder for base payload (without definition) | ||
| item_payload = fab_item_util.build_item_payload( | ||
| item, definition=None, description="Imported from fab" |
There was a problem hiding this comment.
@copilot both definition=None and description="Imported from fab" are the default values, no need to explictly pass them
There was a problem hiding this comment.
Removed explicit definition=None and description="Imported from fab" parameters since they're defaults.
Commit: b8af1ca
| # Item types that don't support definition payloads | ||
| unsupported_types = ( | ||
| ItemType.LAKEHOUSE, | ||
| ItemType.WAREHOUSE, | ||
| ItemType.ML_EXPERIMENT, | ||
| ItemType.ML_MODEL, | ||
| ItemType.DASHBOARD, | ||
| ItemType.PAGINATED_REPORT, | ||
| ItemType.SQL_ENDPOINT, | ||
| ItemType.MIRRORED_WAREHOUSE, | ||
| ItemType.DATAMART, | ||
| ItemType.ENVIRONMENT, # Environment has custom handling | ||
| ) |
- Move unsupported types tuple to fab_constant.UNSUPPORTED_DEFINITION_ITEM_TYPES - Update definition_format_mapping to return simple format strings instead of query params - Notebook default: "fabricGitSource" (not "ipynb") - SparkJobDefinition default: "SparkJobDefinitionV1" - Notebook .py format: None (no format key) - Notebook .ipynb format: "ipynb" - Simplify build_item_payload logic to directly set format on definition dict - Remove explicit default parameters from function calls - Add test for notebook default format behavior Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>
| args.output_batch['names'].append(item.name) | ||
| args.output_batch["items"].append(data) | ||
| args.output_batch["names"].append(item.name) |
There was a problem hiding this comment.
@copilot there are lots of changes you made for formatting. use black formatter and avoid and revert all changes of quotes (' to " you did) and spaces.
There was a problem hiding this comment.
Reverted unwanted formatting changes and applied black with --skip-string-normalization:
- Reverted file to state before formatting commit (preserving single quotes)
- Applied black with
--skip-string-normalizationflag to preserve quote style - Only fixed trailing spaces and line length issues
- No quote changes from
'to"
Commit: 71aada5
…ng-normalization - Revert fab_fs_mkdir_item.py to state before formatting commit - Apply black with --skip-string-normalization to preserve single quotes - Only fix trailing spaces and line length issues - No quote changes from single to double quotes Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>
📥 Pull Request
✨ Description of new changes
Summary
Eliminates redundant payload construction logic scattered across multiple files by introducing a single
build_item_payload()utility. Previously, adding new item types required updating switch-case statements infab_item.py; now onlycommand_support.yamlneeds modification. The implementation uses an extensible design with configuration-based format handling and unsupported types constant.Changes
Core refactoring:
build_item_payload()infab_item_util.py- Single source of truth for payload construction. Handles common fields (type, description, folderId, displayName) and applies format directly to definition dictUNSUPPORTED_DEFINITION_ITEM_TYPESconstant - new item types work automatically unless explicitly unsupportedinput_formatparameter or default fromdefinition_format_mappingget_payload()fromfab_item.py- Eliminated 60+ lines of switch-case duplication_build_payload→_build_definitioninfab_cmd_import_utils.py- More accurate naming; now delegates to centralized builderConfiguration improvements:
UNSUPPORTED_DEFINITION_ITEM_TYPEStofab_constant.py- Centralized tuple of item types that don't support definition payloadsdefinition_format_mappinginfab_types.py- Returns simple format strings instead of query parameters:"fabricGitSource"(default),Nonefor.pyformat,"ipynb"for.ipynbformat"SparkJobDefinitionV1"(default)Command updates:
fab_fs_import_item.py,fab_fs_mkdir_item.py,fab_fs_cp_item.pyall use centralized builderCode formatting:
--skip-string-normalizationto preserve existing quote styleExample usage:
Testing:
Context
The original design required explicitly adding each item type to
fab_item.pyswitch statements, creating maintenance burden and violating DRY. Default format handling was hardcoded with complex special case logic. This refactoring enables adding new item types by configuration alone with a significantly simplified architecture (~30 lines vs ~70 lines) that:fab_constant.pyandfab_types.py)--skip-string-normalizationflag✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.