trustfall icon indicating copy to clipboard operation
trustfall copied to clipboard

Improve onboarding & adding more documentation for `trustfall_core`

Open ginger51011 opened this issue 2 years ago • 8 comments

This is a really interesting project and I'd love to help!

However, right now, the documentation is lacking, or simply just not there. Luckily there are tons of demos, which is always great!

I have started working a bit (and I mean a bit) on my branch here, just adding a bit of links and explanation of types here and there. Should we perhaps add a wish-list for what to document? And is there any recommended reading for this? I found this on the cargo-semver-checks repo, but there might be more?

ginger51011 avatar Feb 09 '23 10:02 ginger51011

I've given this a bit of thought, and here's what I'd propose.

Let's make a new crate called trustfall, and re-export the core functionality that:

  • is needed to run queries, or implement an adapter, and
  • we consider at least somewhat stable.

Then, we can ensure everything in that new crate is extremely nicely documented.

This allows us several things:

  • We can clearly indicate which functionality is somewhat stable, so that downstream users can start building on top of it without worrying too much that they'll be broken in the next version. If anyone is using trustfall_core, then they must be using some of the more advanced, less polished features and would know to expect less stability as a result.
  • We can provide more convenient, sensible, longer-term-stable import paths. The ones in trustfall_core are kind of all over the place because that is just not something I was worrying about at the time.
  • Since everything that is re-exported in the new crate is user-facing and intended to be somewhat stable, it's implicitly the documentation "wish list" as well.

Any new onboarding materials for using the project (either for running queries or for writing adapters) would also use the new trustfall crate instead of directly importing from trustfall_core.

What do you think? I plan on starting this work tomorrow if you don't have any objections.

obi1kenobi avatar Feb 11 '23 01:02 obi1kenobi

Also, I think you've done an exceptionally deep search, and it seems like you've found just about everything there is to find :)

The semantics of Trustfall are very similar to the semantics of GraphQL compiler, which is another Apache2.0-licensed open-source project I worked on previously. Some of the notable additions in Trustfall relative to what the older GraphQL compiler project could do are listed in this file: https://github.com/obi1kenobi/trustfall/blob/main/spec.md

obi1kenobi avatar Feb 11 '23 01:02 obi1kenobi

I think you're on the right track. This new trustfall crate could simply be an interface, and trustfall_core could be a library hiding the magic.

One thing I think we should be careful about though is adding too much, or making the ecosystem too convoluted. Right now this repo is quite full, with all demos being located directly at the top (maybe move them to a demo/?), and multiple crates in the same repo (this is just personal preference though, and it might be nice to have it all here honestly). It's always nice for people to have a one-stop shop, and not having to jump between crates.

But yes, most of my intial confusion probably came because all examples included schema.graphql, and you could also find files named graphql_* in trustfall_core, which quite naturally leads to the conclusion that they are were closely bundled.

ginger51011 avatar Feb 11 '23 11:02 ginger51011

One thing that came to mind might be the inclusion of parsers. Right now the graphql_* things are part of trustfall_core, but maybe parsers should be decided by feature-flags? Or perhaps other features that might not be for everyone.

In fact, you could probably keep trustfall_core as it is, and only include more advanced or instable features if the user asks for them. Of course, this could be the case with an interface trustfall crate as well, only re-exporting things when users ask for them. Here I think you have enough knowledge about the system to say what's best.

Cargo book on feature flags for reference

ginger51011 avatar Feb 11 '23 13:02 ginger51011

One thing I think we should be careful about though is adding too much, or making the ecosystem too convoluted. Right now this repo is quite full, with all demos being located directly at the top (maybe move them to a demo/?)

Definitely open to moving them to demo/! Happy to do it or to let you do it. It was your good idea so I'll leave you the option to choose whether to implement it yourself or have me do it.

only include more advanced or instable features if the user asks for them

Right now, we only have one parser+frontend combination, and just about everything in trustfall_core is needed by something in order to make queries execute. Even if the adapter is provided by one crate and the query-execution code is called by another, both components need to be compiled in.

Definitely good to keep features in mind as a possibility. Right now, I think it's a bit early, though.

obi1kenobi avatar Feb 11 '23 14:02 obi1kenobi

(This should probably have its own issue)

Perhaps we could consider moving it to trustfall_core/examples instead? It would need some reorganizing (but nothing major). The cargo book has some information.

This way one could run examples directly from trustfall_core by running

cargo run --example hackernews -- examples/hackernews/example_queries/<whatever>

or something like that. It all depends if the trustfall_core is to remain a source of truth or if you want to go ahead with trustfall as a well-documented interface instead. What do you think?

ginger51011 avatar Feb 11 '23 15:02 ginger51011

Oh, I love that idea! Let's move them to trustfall/examples though.

After thinking some more, I think it's important to allow the underlying code to move quickly without breaking downstream users. The best way to do that is to use the trustfall crate as the thing everyone depends on when they need stability + using the "semver trick" to keep major version bumps of trustfall_core from breaking everyone all the time.

obi1kenobi avatar Feb 11 '23 15:02 obi1kenobi

Speaking of examples... In #149, in addition to making the HN example adapter use BasicAdapter, I think I might have finally figured out one sensible and common pattern to extract much of the boilerplate out of writing adapters. The inner generic function inside two (could have been three, I just didn't bother) of the four adapter methods is actually completely independent of the adapter, and can be provided as an external helper.

obi1kenobi avatar Feb 12 '23 05:02 obi1kenobi