Skip to content

Accept both str and Path types for functions in databricks/sdk/service/files.py#740

Open
fpgmaas wants to merge 2 commits intodatabricks:mainfrom
fpgmaas:feature/path-for-files
Open

Accept both str and Path types for functions in databricks/sdk/service/files.py#740
fpgmaas wants to merge 2 commits intodatabricks:mainfrom
fpgmaas:feature/path-for-files

Conversation

@fpgmaas
Copy link

@fpgmaas fpgmaas commented Aug 29, 2024

Changes

Currently, the functions in databricks/sdk/service/files.py accept only str for arguments that are paths. This can lead to confusing behavior for end-users when passing in a Path object.

Tests

I parametrized the unit tests.

  • make test run locally
  • make fmt applied
  • relevant integration tests applied

Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Hi @fpgmaas, thanks for contributing this change to the SDK! Unfortunately, the code you're modifying is autogenerated and would be overwritten in the next SDK release if merged. Implementing this would require a change in our codegen tool itself. Can I ask you to make a feature request instead?

So that I understand the rationale: "This can lead to confusing behavior for end-users when passing in a Path object." The interfaces for these methods all specify that they accept strings. Can users just call str(...) themselves before using these methods?

@fpgmaas
Copy link
Author

fpgmaas commented Sep 16, 2024

Hi @fpgmaas, thanks for contributing this change to the SDK! Unfortunately, the code you're modifying is autogenerated and would be overwritten in the next SDK release if merged. Implementing this would require a change in our codegen tool itself. Can I ask you to make a feature request instead?

Thanks for your response. I overlooked the comment # Code generated from OpenAPI specs by Databricks SDK Generator. DO NOT EDIT. at the top, my bad! I opened the issue #762.

Can users just call str(...) themselves before using these methods?

Yes, that will definitely work. However, the error that is thrown is not very clear which makes it a bit annoying to debug. So for a better user experience I think allowing Path objects to the path argument would be a nice improvement.

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.

2 participants