-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: centralize item payload construction with simplified format handling #23
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
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot this is the default value, no need to specify it explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed explicit description="Imported from fab" parameter since it's the default value.
Commit: b8af1ca
| } | ||
| # 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot defined this in fab_constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved unsupported types tuple to fab_constant.UNSUPPORTED_DEFINITION_ITEM_TYPES.
Commit: b8af1ca
|
|
||
| # If no definition provided, return base payload (used by mkdir) | ||
| if definition is None: | ||
| return base_payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot instead of this do
if definition:
# Check if item type is not supported
if item_type in unsupported_types:
from fabric_cli.errors import ErrorMessages
raise FabricCLIError(
ErrorMessages.Hierarchy.item_type_doesnt_support_definition_payload(
str(item.item_type)
),
fab_constant.ERROR_UNSUPPORTED_COMMAND,
)
if input_format:
definition["format"] = input_format
else:
default_format = definition_format_mapping.get(item_type, None)
if default_format:
definition["format"] = default_format
base_payload["definition"] = definition
return base_payloadThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot and ehnace the definition_format_mapping to return default values. for notebook it is fabricGitSource, for SparkJobDefinition, it is SparkJobDefinitionV1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhanced definition_format_mapping to return default values:
- Notebook: "fabricGitSource" (default), None for .py format, "ipynb" for .ipynb format
- SparkJobDefinition: "SparkJobDefinitionV1"
Commit: b8af1ca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the logic as suggested:
- Check if definition exists, then validate item type
- Apply format from
input_formator default fromdefinition_format_mapping - Set format directly on definition dict
- Enhanced
definition_format_mappingwith simple format strings:- Notebook default: "fabricGitSource"
- SparkJobDefinition default: "SparkJobDefinitionV1"
Commit: b8af1ca
- 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>
📥 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 builderExample 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)✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.