-
Notifications
You must be signed in to change notification settings - Fork 4k
Update docstrings for st.connection #6528
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
Update docstrings for st.connection #6528
Conversation
23900ac to
5cc7cd0
Compare
b3e2540 to
d20d948
Compare
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
|
I think this will need a couple final tweaks once #6530 is merged to the feature branch |
93ae79b to
dbecf31
Compare
|
OK, this is ready for a final review and merge IMO. |
* 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>
📚 Context
Working on st.experimental_connection
What kind of change does this PR introduce?
🧠 Description of Changes
Improving docstrings for st.connection
🧪 Testing Done
🌐 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.