Skip to content

Conversation

@pietern
Copy link
Contributor

@pietern pietern commented Jun 30, 2022

First pass to ensure better completeness of the arguments to the methods in service.py.

@pietern pietern requested review from alexott, fjakobs and nfx June 30, 2022 09:16
Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we minimize the changes in the signatures? Or we need to bump version & declare it as a breaking change

@pietern
Copy link
Contributor Author

pietern commented Jul 1, 2022

Thanks for taking a look @alexott.

Good points regarding kwarg order. I did some investigation:

  1. The databricks-api project wraps the different *Service classes and doesn't modify the method calls. This means it would break if users of databricks-api call into these methods with positional arguments.
  2. Earlier changes to this same file have changes kwarg order (or inserted kwargs), leading to the same issue (precedent).

I think we should consider stable named parameter ordering at 1.0.

For now, I think we're safe to call it out in the release notes and bump to 0.18.

pietern added 2 commits July 4, 2022 09:56
Ran `black -S --line-length 100`.
@pietern
Copy link
Contributor Author

pietern commented Jul 4, 2022

@alexott

I did some more investigation and found a couple call sites that use positional parameters and would be broken by this change. Unfortunately we cannot start forcing keyword arguments (e.g. with * separator argument) or we'd end up breaking these call sites. To address this I added a unit test to explicitly test for positional argument ordering in #507 and use the output of this test to generate service.py.

All new keyword arguments are appended so none of the existing call sites should break.

@pietern pietern requested a review from alexott July 4, 2022 10:29
@pietern
Copy link
Contributor Author

pietern commented Jul 4, 2022

@nfx Filed follow ups for the issues not in scope here.

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2022

Codecov Report

Merging #505 (0684aa1) into main (dedc3c8) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #505      +/-   ##
==========================================
- Coverage   61.10%   61.08%   -0.02%     
==========================================
  Files          55       55              
  Lines        4664     4662       -2     
==========================================
- Hits         2850     2848       -2     
  Misses       1814     1814              
Impacted Files Coverage Δ
databricks_cli/pipelines/cli.py 96.34% <0.00%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dedc3c8...0684aa1. Read the comment docs.

@pietern pietern merged commit 0b6a78e into databricks:main Jul 5, 2022
@pietern pietern deleted the service-gen branch July 5, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants