Skip to content

Conversation

@sfc-gh-jcarroll
Copy link
Contributor

📚 Context

Working on st.experimental_connection

  • What kind of change does this PR introduce?

    • Docstrings

🧠 Description of Changes

Improving docstrings for st.connection

  • ExperimentalBaseConnection
  • Snowpark
  • (coming soon) SQL
  • (coming soon) connection_factory

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-jcarroll sfc-gh-jcarroll changed the title Update docstrings Update docstrings for st.connection Apr 20, 2023
@vdonato vdonato self-requested a review April 20, 2023 22:22
@vdonato vdonato added the security-assessment-completed Security assessment has been completed for PR label Apr 20, 2023
@vdonato vdonato force-pushed the feature/st.experimental_connection branch from 23900ac to 5cc7cd0 Compare April 20, 2023 23:55
@vdonato vdonato force-pushed the connection-docstrings branch from b3e2540 to d20d948 Compare April 21, 2023 00:56
@snehankekre snehankekre self-requested a review April 21, 2023 04:20
Copy link
Contributor

Choose a reason for hiding this comment

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

@sfc-gh-jcarroll Johannes had requested adding callouts to the effect of "don’t create this class directly but use st.connection etc" to some of these classes. Let's add a sentence to all such places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it's covered by the line above

Initialize using st.experimental_connection("<name>", type="sql").

And we have the same thing on Snowpark.

@vdonato what do you think? Could make it even more explicit but not sure it's needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd be useful to be extra explicit about calling this out. My initial thought was that this could be mentioned as an additional point in the documentation site but not in the docstring, but I think the current doc prototypes mostly render the docstrings nicely, so it'd be good to just include this here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's the update look?

@sfc-gh-jcarroll
Copy link
Contributor Author

I think this will need a couple final tweaks once #6530 is merged to the feature branch

@sfc-gh-jcarroll
Copy link
Contributor Author

OK, this is ready for a final review and merge IMO.

@vdonato vdonato merged this pull request into streamlit:feature/st.experimental_connection Apr 21, 2023
vdonato pushed a commit that referenced this pull request Apr 21, 2023
* Update docstrings for base connection and snowpark connection

* Add SQL docstring updates

* Add connection_factory docstrings

* Fix reStructuredText issues in connection_factory docstrings

* Fix reStructuredText issues in ExperimentalBaseConnection docstrings

* Fix reStructuredText issues in Snowpark docstrings

* Fix reStructuredText issues in SQL docstrings

* Address PR feedback

* Post-rebase cleanups

* More cleanups from omnibus changes

* Add explicit guidance to not initialize connections directly

* Add example and fix note formatting on snowparkconnection.session

* Formatting fixes

---------

Co-authored-by: snehankekre <snehan.minerva@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants