rename camelCase params to snake_case with deprecation shims#267
rename camelCase params to snake_case with deprecation shims#267domino-blake wants to merge 9 commits into
Conversation
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
There was a problem hiding this comment.
The **kwargs is purely the deprecation mechanism — every method that has it is using it to absorb the old camelCase name without breaking existing callers
|
|
||
| - 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.
fixed by moving the filtering rules into check_snake_case.py itself, so both CI and pre-commit invoke the same 9987404
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.
There was a problem hiding this comment.
Added a note in CHANGELOG.md, README.md, and README.adoc explicitly calling out that these renames are Python-side only bd1785b
…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
force-pushed
the
blake/pr-renames
branch
from
f996a64 to
3402e8a
Compare
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