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

Add basic reference docs for createHttpClient #918

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scotttrinh
Copy link
Collaborator

We probably want to revisit the options docs, but at the very least, this adds documentation for the http client creation function.

Copy link
Contributor

@raddevon raddevon left a comment

Choose a reason for hiding this comment

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

Looking this over again at the end and comparing to the createClient docs above it, I believe there's only one very minor change in here for you, but this may have revealed a couple of bugs in the new docs site.

Creates an HTTP client for interacting with an EdgeDB instance from a browser or edge runtime environment. This function returns an instance of :js:class:`Client` configured to communicate over HTTP using the Fetch API.

:param options:
This is an optional parameter. When it is not specified the client
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is an optional parameter. When it is not specified the client
This is an optional parameter. When it is not specified, the client

Comment on lines +164 to +181
:param boolean options.tlsSecurity:
Determines whether certificate and hostname verification is enabled.
Valid values are ``'strict'`` (certificate will be fully validated),
``'no_host_verification'`` (certificate will be validated, but
hostname may not match), ``'insecure'`` (certificate not validated,
self-signed certificates will be trusted), or ``'default'`` (acts as
``strict`` by default, or ``no_host_verification`` if ``tlsCAFile``
is set).

The above connection options can also be specified by their corresponding
environment variable. If none of ``dsn``, ``credentialsFile``, ``host`` or
``port`` are explicitly specified, the client will connect to your
linked project instance, if it exists. For full details, see the
:ref:`Connection Parameters <ref_reference_connection>` docs.


:param number options.timeout:
Connection timeout in milliseconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this got rendered as you intended, but I'm not sure if I'd call this a bug in the docs site, or if we just need to rework the documentation.

CleanShot 2024-03-25 at 11 12 33@2x

We get another "Arguments" heading before options.timeout. I think we would just want to continue the arguments list after the paragraph about connection options without another heading. I can't think of a case where we would want the current behavior, but I wanted to check in with you and see what you think. I guess if there's an easy fix we can make here we should do that. I'm not sure there is though.

The easy "fix" would be indenting it, maybe as a note inside the description of one of the params, but it's relevant to multiple params so I don't think that really works here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FWIW, all of this is copied wholesale from the createClient docs, so any issues here are also issues there, I'd imagine. Definitely good with taking the time to update these docs both in layout and content though!

Comment on lines +123 to +125
will connect to the current EdgeDB Project instance.

If this parameter is a string it can represent either a
Copy link
Contributor

Choose a reason for hiding this comment

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

The newlines here get collapsed. In the rendered output, "If" is directly against the preceding period with nothing between. I've started a thread about it on Slack. Seems like a bug in the docs.

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.

None yet

2 participants