Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark looker_sdk as an optional dependency #151

Merged
merged 5 commits into from
Sep 11, 2024

Conversation

sfc-gh-cnivera
Copy link
Collaborator

@sfc-gh-cnivera sfc-gh-cnivera commented Sep 10, 2024

Some users may not want to run through partner flows, in which case installing the Looker SDK is unnecessary. Following the discussion in #150 (comment), this PR moves looker-sdk into the "extra" dependencies section, meaning it won't be installed unless specified (e.g. poetry install -E looker).

To support this, I made some small refactors to only import looker conditionally. The popup will now be triggered if the user attempts to click the "Start with partner semantic model" button.

Shown below is a video showing that without looker installed, the rest of the app is functional, and a warning pops up if a user attempts to use the Looker integration without the SDK.

CleanShot.2024-09-10.at.14.30.06.mp4

@sfc-gh-cnivera sfc-gh-cnivera marked this pull request as ready for review September 10, 2024 22:36
Copy link
Collaborator

@sfc-gh-twhite sfc-gh-twhite left a comment

Choose a reason for hiding this comment

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

This LGTM! Awesome job, I tested this extra install using pip and all seems to work well.

One non-blocking note - should we add a small block to the README to indicate that if a user wishes to leverage Looker (insert fancy words as to why one would want to use this solution with Looker)?

Note

To use the Looker features, you will need to install the extra.
pip install -e ".[looker]"

@sfc-gh-jsummer
Copy link
Collaborator

@sfc-gh-twhite - To your point about adding a note for install, we should have a partner semantic tool section in the updated README that contains such information and instructions. I can write when we are ready.

@sfc-gh-cnivera
Copy link
Collaborator Author

Good call @sfc-gh-twhite, stubbed out a small section with those instructions! We can definitely flesh out this section in a subsequent PR @sfc-gh-jsummer.

@sfc-gh-cnivera sfc-gh-cnivera merged commit d6d3972 into main Sep 11, 2024
3 checks passed
@sfc-gh-cnivera sfc-gh-cnivera deleted the cnivera/looker-as-extra branch September 11, 2024 17:22
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