Skip to content

Refactoring#40

Merged
kravets-levko merged 4 commits into
databricks:mainfrom
kravets-levko:refine-code
Aug 2, 2022
Merged

Refactoring#40
kravets-levko merged 4 commits into
databricks:mainfrom
kravets-levko:refine-code

Conversation

@kravets-levko
Copy link
Copy Markdown
Contributor

  1. Get rid of additional wrapper for client and merge DBSQLClient and HiveClient classes
  2. Rename some of other Hive** classes
  3. Update docs and tests
  4. Use import everywhere to get as much type hinting as possible

Copy link
Copy Markdown

@susodapop susodapop left a comment

Choose a reason for hiding this comment

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

LGTM.

How can we get the tests check to pass?

@kravets-levko
Copy link
Copy Markdown
Contributor Author

@susodapop thanks!

How can we get the tests check to pass?

Currently no way for external PRs 😢 I still make sure they pass locally, though

@moderakh
Copy link
Copy Markdown
Contributor

moderakh commented Aug 1, 2022

  1. Get rid of additional wrapper for client and merge DBSQLClient and HiveClient classes
  2. Rename some of other Hive** classes

@kravets-levko could you please explain the rationale of why this is needed? (I am not saying it is not needed, I just would like to have more context the rationale behind the proposed change :-) )

also is there a jira ticket for this? could you please mention the PECO jira ticket number for this work?

@kravets-levko
Copy link
Copy Markdown
Contributor Author

@moderakh I created https://databricks.atlassian.net/browse/PECO-216 some time ago for this. There were few reasons for these changes:

  1. at the very beginning we agreed that there are too much "hive" stuff here, so I took an opportunity to rename some classes. There's still a HiveDriver class, but I decided to keep it for now;
  2. DBSQLClient was just a very thin wrapper to make HiveClient easier to use and ensure that all required parameters (e.g. PAT) are passed. Now, when we have an idea where it goes, we don't need that wrapper and could update HiveClient itself;
  3. Using import instead of require is just for better TS support: for some reason, TS doesn't pick up types for modules imported with require 🤷‍♂️
  4. DBSQLClient.close is now promisified just for consistency with other methods.

@kravets-levko kravets-levko merged commit a2a4b32 into databricks:main Aug 2, 2022
@kravets-levko kravets-levko deleted the refine-code branch August 2, 2022 20:07
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.

3 participants