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

WIP: Add documentation for the Adapter trait #142

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ginger51011
Copy link
Contributor

I think it's important that this trait in particular is well-documented. Right now I'm working based on demos, but I chose to relate it to GraphQL since it's currently the query language of choice.

I might be a bit off with descriptions here, and I think the query_hint and vertex_hint may need more explanations, since they are ignored in the demos from what I can see. But according to #131 these are subject to change soonTM 🛩️

@obi1kenobi
Copy link
Owner

Tbh I'm thinking of completely revamping this trait.

#131 will remove the explicit "hint" arguments in favor of a &QueryInfo value. If we then adopt the (in my mind, better) nomenclature from BasicAdapter, then there's basically nothing left here that won't get changed.

What do you think? Are the names of BasicAdapter better and easier to understand? If we were to adopt them here in a breaking change, would that be an improvement?

@ginger51011
Copy link
Contributor Author

Sorry about the CI, have added "editor.formatOnSave": true now...

I have tried, based on demo-hackernews, to provide adequate examples and explanations for some of the adapter methods. However, I might be off at some places. But perhaps it is a good idea for a complete newbie to attempt to write the docs, because it makes it painfully obvious what is hard to understand...

--- Above this line was before you posted your comment :)

Right now I think this trait is quite difficult to understand, but looking at BasicAdaptor I realize that I should have looked there first!

If nothing else, BasicAdapter has a lot better method names, and just that makes it easier to use. It might be worth a breaking change, because right now the demos just ignore query_hint and vertex_hint, which makes them basically magic.

For me at least, with very limited background in graph theory and graph databases, but some knowledge of GraphQL, I think it might be helpful to explain concepts in the adaptors by describing them in a GraphQL context (like "a starting vertex is like a field of the Query type in a GraphQL schema"), like I have tried to do in this PR.

@obi1kenobi
Copy link
Owner

obi1kenobi commented Feb 10, 2023

Right now I think this trait is quite difficult to understand, but looking at BasicAdaptor I realize that I should have looked there first!

This is the fault of insufficient docs 😅 Let's try to figure out how to best direct the next person to that easier-to-implement trait. It's easier to implement because it can't do everything that the "real" one under the hood can do, but it's enough for most cases.

If nothing else, BasicAdapter has a lot better method names, and just that makes it easier to use. It might be worth a breaking change, because right now the demos just ignore query_hint and vertex_hint, which makes them basically magic.

Yes, agreed. I realized a design like #131 is necessary because the hint parameters are technically all you need but in practice are just waaay too tedious to use. After I've put in a few more iterations on #131, I'd love your help and feedback there on naming and documentation. It's super useful to have a second pair of eyes on these things — thanks for taking the time!

I think it might be helpful to explain concepts in the adaptors by describing them in a GraphQL context (like "a starting vertex is like a field of the Query type in a GraphQL schema"), like I have tried to do in this PR.

I understand the sentiment here. I have two concerns, and a concrete proposal which I'll name first.

I think we may be better off with an "Learning Trustfall for GraphQL users" page somewhere in the docs, instead of inlining the GraphQL analogies and comparisons into the main documentation.

Here's why:

  • Trustfall isn't actually GraphQL: it uses an analogous syntax for the most part, but its semantics are quite different and more powerful. When seeing the syntax, people very familiar with GraphQL are already tempted to (incorrectly) say "ah, this is just another GraphQL federation implementation" -- the briefest of looks at the HackerNews thread on Trustfall is sufficient to confirm this. We need to discourage that leap as hard as we can, rather than encouraging it by comparing and analogizing to GraphQL across the docs.
  • People not familiar with GraphQL shouldn't have to learn it first before understanding our docs. Trustfall needs to be friendly to people from a SQL background, or Excel background, or dataframes background. Explaining Trustfall concepts by comparing them with analogous GraphQL concepts makes GraphQL a "preferred" background for learning Trustfall, which not only doesn't help everyone else but actively hurts: "oh no, another thing I have to learn before I can understand this."

By having documentation free of concepts from other languages, and having separate "Learning Trustfall for X users" pages in the docs, we can let the readers self-select into the right groups and then we can make the right analogies to them:

  • In the SQL case: "The @optional directive is like a SQL LEFT JOIN: it means it's okay if the data on the other side doesn't exist. In that case, any selections from inside the @optional block have a value of null."
  • In the GraphQL case: "A starting vertex is one available as a field on the Query type of the schema."
    etc.

What do you think?

@ginger51011
Copy link
Contributor Author

Yes, agreed. I realized a design like #131 is necessary because the hint parameters are technically all you need but in practice are just waaay too tedious to use. After I've put in a few more iterations on #131, I'd love your help and feedback there on naming and documentation. It's super useful to have a second pair of eyes on these things — thanks for taking the time!

I'd be happy to help! Right now I'm looking intro using Trustfall myself, so I'm very interested in seeing improvements.

I think we may be better off with an "Learning Trustfall for GraphQL users" page somewhere in the docs, instead of inlining the GraphQL analogies and comparisons into the main documentation.

Reading your response I would say I agree. I think the reason I have to reason around it like that myself is that when there is nothing to latch onto, it is much easier to map it to something you already know.

If anything, I think the docs (or README, or whatever) could use some images of operations. I saw some on your blog, but they don't explain in much detail what actually happens inside the adapters/engine, and how that would be different from GraphQL. This might be just me though.

Another question is: How much does one want to steer the adapter implementations? Looking at the demos, there are definitely patterns that occurs several times. Are all users to write their own implement_property! when most of them look the same? Could there be room for a trustfall_adapter_macros (or is there one that I have missed?)

  • In the SQL case: "The @optional directive is like a SQL LEFT JOIN: it means it's okay if the data on the other side doesn't exist. In that case, any selections from inside the @optional block have a value of null."
  • In the GraphQL case: "A starting vertex is one available as a field on the Query type of the schema."

Just those two are very useful: Take something the users know, and explain new concept with old ones. Perhaps we shouldn't explain things using only GraphQL, but implementing these in the docs (where people go first) might come a long way.

@obi1kenobi
Copy link
Owner

obi1kenobi commented Feb 10, 2023

If anything, I think the docs (or README, or whatever) could use some images of operations. I saw some on your blog, but they don't explain in much detail what actually happens inside the adapters/engine, and how that would be different from GraphQL. This might be just me though.

The README definitely needs several layers' worth of rework. Some images would definitely be nice, but I think some of the "getting started" docs might be more useful now that many people are already at least somewhat familiar with the project and what it does.

Ultimately it's just a question of sequencing the work in maximum-impact order. I think good images are a bit difficult to make, and probably not the immediate highest priority compared to "getting started" docs that point to BasicAdapter instead of Adapter, for example.

Another question is: How much does one want to steer the adapter implementations? Looking at the demos, there are definitely patterns that occurs several times. Are all users to write their own implement_property! when most of them look the same? Could there be room for a trustfall_adapter_macros (or is there one that I have missed?)

I've started several drafts of macros, and never quite found a design I loved. It's really unfortunate that macros are not allowed to generate a "match arm," because that might have been nice.

I'll continue thinking about this. If you end up looking at the problem and find a possible design something you like, please bring it up in an issue or draft PR!

@obi1kenobi
Copy link
Owner

I'd also love to know about the domain where you're thinking about using Trustfall, if you'd be up for a chat? I hang out on the rust-lang Zulip and you can also find me on Discord if you'd prefer that. No pressure, obviously.

@obi1kenobi obi1kenobi added the A-docs Area: documentation for the query language or library APIs label Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for the query language or library APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants