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

Adds features implemented to docs usage #104

Merged
merged 25 commits into from
Nov 29, 2023
Merged

Adds features implemented to docs usage #104

merged 25 commits into from
Nov 29, 2023

Conversation

seanparkross
Copy link
Contributor

@seanparkross seanparkross commented Oct 23, 2023

Includes features which this NDC implements and links to docs for each.

Inline with the ndc-connector-template

@seanparkross seanparkross self-assigned this Oct 23, 2023
@seanparkross seanparkross changed the title WIP: Adds info to docs and root readme Adds info to docs and root readme Oct 23, 2023
@seanparkross seanparkross marked this pull request as ready for review October 23, 2023 18:01
Copy link
Contributor

@soupi soupi left a comment

Choose a reason for hiding this comment

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

Looks good. I have some comments.

I do have a question about why we are adding this information here. It seems to talk about a higher level of abstraction, which is GraphQL API for all connectors which implement ndc-spec, and is not specific for this connector.

Perhaps this should go in a central place and be linked from this repository (and other connectors' repository) instead?

docs/usage/queries/simple-queries.md Outdated Show resolved Hide resolved
docs/usage/queries/simple-queries.md Outdated Show resolved Hide resolved
@@ -0,0 +1,112 @@
# Use Multiple Arguments in a Query
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it unfortunate that the term arguments is overloaded and may also refer to collection arguments.

I would actually suggest that this section is unnecessary, because we've already showed in simple-queries.md that we can use multiple arguments (limit and offset).
Another suggestion is to fold this into simple queries.
wydt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Can we use the term "query arguments"?
  2. I don't mind it that much. I think it's handy to be explicit (especially for the AI bot which is becoming the de-facto method of using docs) The simple queries page is fairly long already. If you have very strong feelings about this Gil we can change it. CC: @robertjdominguez

Comment on lines 5 to 6
If multiple queries are part of the same request, they are executed **sequentially**, the individual responses are
collated and returned together. You can fetch objects of different unrelated types in the same query.
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
If multiple queries are part of the same request, they are executed **sequentially**, the individual responses are
collated and returned together. You can fetch objects of different unrelated types in the same query.
You can fetch objects of different unrelated types in the same query.

Should we include implementation details here if they are not semantically meaningful? There's no reason why we can't run the queries in parallel, or run them all in a single query, and keep the same semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this something that would need to change with the connector to implement?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it is entirely the connector's decision how to implement this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepted suggestion

docs/usage/queries/pagination.md Outdated Show resolved Hide resolved
docs/usage/queries/sorting.md Outdated Show resolved Hide resolved
docs/usage/queries/sorting.md Outdated Show resolved Hide resolved
docs/usage/queries/sorting.md Outdated Show resolved Hide resolved
}
```

## List based search operators (_in, _nin)
Copy link
Contributor

Choose a reason for hiding this comment

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

_nin does not currently exist in ndc-spec, only _in. I think using _not over _in does the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated. Can you check that it's correct in the second example.

@@ -0,0 +1,137 @@
# PostgreSQL: Filter based on computed fields
Copy link
Contributor

Choose a reason for hiding this comment

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

Computed fields are currently not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted and removed from indices

@seanparkross
Copy link
Contributor Author

seanparkross commented Oct 24, 2023

Hey Gil :)

I do have a question about why we are adding this information here. It seems to talk about a higher level of abstraction, which is GraphQL API for all connectors which implement ndc-spec, and is not specific for this connector.

The decision was made (after much back and forth, analysis and conversation), that all connector specific docs, including GraphQL API docs, would live in their respective repos... because:

  • All connectors will enable slightly different features and functionality. Managing those differences, however slight (especially the slight differences), in a central place has proven to be difficult in v2 and would be predicted to become more so in v3 with the intended (high) number of connectors which will eventually be added.

  • Developers would need to update two places to update their connector and the docs for them. With approval processes and timelines it would lead to out of sync features and docs.

  • We want to set a precedent and common format for how the public will document their community connectors, in line with how we document ours.

  • Connectors will likely also at some point enable other APIs besides GraphQL (REST, gRPC). Some may do one and not the other. Managing these differences in a central place would become difficult.

  • If all in-repo connector docs follow a predictable and common template then they should be easy to follow across multiple repos.

  • If the connector hub decided to render the docs for each connector from the repo then this methodology can enable that.

Once this PR is approved and this repo is public, we will remove the PostgreSQL GraphQL API docs from the DDN Docs site.

CC: @robertjdominguez @rahulagarwal13 @manasag @codedmart

@danieljharvey danieljharvey force-pushed the update-readme-and-docs branch from 4c4ae7a to ddb4d2e Compare November 1, 2023 11:31
@seanparkross seanparkross changed the title Adds info to docs and root readme Adds features implemented to docs usage Nov 29, 2023
@soupi soupi enabled auto-merge November 29, 2023 07:28
@soupi soupi added this pull request to the merge queue Nov 29, 2023
Merged via the queue into main with commit da9af5a Nov 29, 2023
26 checks passed
@soupi soupi deleted the update-readme-and-docs branch November 29, 2023 07:37
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