rename camelCase params to snake_case with deprecation shims#267
rename camelCase params to snake_case with deprecation shims#267domino-blake wants to merge 6 commits intoblake/pr-qualityfrom
Conversation
…both theheading and parameter list - Documented all 7 parameters (including the 4 that were already there but undocumented: environment_id, external_volume_mount_ids, commit_id, branch, app_id) - Added code examples showing the three common new use cases (branch, commit, specific app ID) - Added a deprecation note pointing callers from the old camelCase names to the new ones - Updated app_unpublish() to document its app_id parameter (which was also previously undocumented) - Updated app_publish() to include branch and commit_id when publishing
domino-blake
changed the title
|
Once #265 merges, need to change this to target master |
|
issue: The table says endpoint_publish renames modelId/modelVersionId, but the actual diff renames commitId to commit_id. Looks like the table just needs a small fix. |
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| is_direct = kwargs.pop("isDirect") |
There was a problem hiding this comment.
issue: If someone passes both is_direct=... and isDirect=... in the same call, the old name silently wins here because kwargs gets popped after binding. Might be worth raising a ValueError when both are provided so nobody gets surprised.
| tier=None, | ||
| publishApiEndpoint=None, | ||
| publish_api_endpoint=None, | ||
| **kwargs, |
There was a problem hiding this comment.
question: What's the idea behind adding kwargs here? it looks like the method only uses well known names. We should probably adopt that instead.
if it's just to deal with parameter naming, then i'd suggest putting a deprecation warning and do a full replace in the next release
|
|
||
| - name: snake_case | ||
| run: | | ||
| find domino -name "*.py" \ |
There was a problem hiding this comment.
suggestion: The find/grep combo here seems different than what the pre-commit hook does
There was a problem hiding this comment.
note: Just want to call out that these renames are Python-side only -- the JSON keys going over the wire are still camelCase. Might be worth a quick note in the README or changelog so users don't think the API contract changed.
2 participants
What does this PR do?
Renames 18 public API parameters from camelCase to snake_case to comply with PEP 8.
Old names continue to work and emit a
DeprecationWarningpointing callers to thenew name — no hard breaking changes in this PR.
Renamed parameters
runs_start/runs_start_blockingisDirectis_directruns_start/runs_start_blockingcommitIdcommit_idruns_start/runs_start_blockingpublishApiEndpointpublish_api_endpointrun_stopsaveChangessave_changesruns_statusrunIdrun_idget_run_logincludeSetupLoginclude_setup_logruns_stdoutrunIdrun_idfiles_listcommitIdcommit_idendpoint_publishmodelId/modelVersionIdmodel_id/model_version_idapp_publishunpublishRunningApps/hardwareTierId/environmentId/externalVolumeMountIds/commitId/appIdTests
tests/test_deprecations.pywith full coverage: every old name raisesDeprecationWarning, every new name is accepted silently.Note for reviewers
These are technically breaking in a future major version (old names will eventually
be removed). The deprecation shims give callers time to migrate. Happy to discuss
the timeline for removal if needed.
Testing
18 new tests in
tests/test_deprecations.pyAll existing tests continue to pass (they use the old names, which still work)
Unit tests passing
Deprecation warnings added for all renamed params
CHANGELOG updated
README updated