docs: Document client interfaces in client/db.go #1305

AndrewSisley · 2023-04-06T17:12:24Z

Relevant issue(s)

Resolves #1303

Description

Adds basic documentation for the client interfaces in client/db.go.

Some oddities are noted in the commit message bodies, but I see changing them as out of scope - happy to tweak the easy ones here if people feel otherwise though.

shahzadlone

Appreciate you taking the time to add documentation. LGTM, pointed some minor typos.

shahzadlone · 2023-04-06T17:22:23Z

client/db.go

@@ -19,30 +19,75 @@ import (
 	"github.com/sourcenetwork/defradb/events"
 )

+// DB is the primary public programatic access point to the local Defra instance.


todo:

Suggested change

// DB is the primary public programatic access point to the local Defra instance.

// DB is the primary public programmatic access point to the local Defra instance.

Cheers :)

programatic => programmatic

shahzadlone · 2023-04-06T17:22:56Z

client/db.go

@@ -19,30 +19,75 @@ import (
 	"github.com/sourcenetwork/defradb/events"
 )

+// DB is the primary public programatic access point to the local Defra instance.


todo:

Suggested change

// DB is the primary public programatic access point to the local Defra instance.

// DB is the primary public programatic access point to the local DefraDB instance.

I guess it should be :)

DefraDB (change all instances of this)

shahzadlone · 2023-04-06T17:23:26Z

client/db.go

@@ -19,30 +19,75 @@ import (
 	"github.com/sourcenetwork/defradb/events"
 )

+// DB is the primary public programatic access point to the local Defra instance.
+//
+// It should be contructed via the [db] package, via the [db.NewDB] function.


todo:

Suggested change

// It should be contructed via the [db] package, via the [db.NewDB] function.

// It should be constructed via the [db] package, via the [db.NewDB] function.

cheers

contructed => constructed

shahzadlone · 2023-04-06T17:24:17Z

client/db.go

 type DB interface {
+	// Store contains Defra database functions protected by an internal, short-lived, transaction, allowing safe


todo:

Suggested change

// Store contains Defra database functions protected by an internal, short-lived, transaction, allowing safe

// Store contains DefraDB functions protected by an internal, short-lived, transaction, allowing safe

Will do

Defra database => DefraDB

shahzadlone · 2023-04-06T17:25:42Z

client/db.go

 	// WithTxn returns a new [client.Store] that respects the given transaction.
 	WithTxn(datastore.Txn) Store

+	// Root returns the underlying root store, within which all data managed by Defra is held.


todo:

Suggested change

// Root returns the underlying root store, within which all data managed by Defra is held.

// Root returns the underlying root store, within which all data managed by DefraDB is held.

shahzadlone · 2023-04-06T17:26:08Z

client/db.go

 	Root() datastore.RootStore
+
+	// Blockstore returns the blockstore, within which all blocks (commits) managed by Defra are held.


todo:

Suggested change

// Blockstore returns the blockstore, within which all blocks (commits) managed by Defra are held.

// Blockstore returns the blockstore, within which all blocks (commits) managed by DefraDB are held.

shahzadlone · 2023-04-06T17:30:09Z

client/db.go

+	// It does not explicitly clear any data from persisted storage, and a new [DB] instance may typically
+	// be created after calling this to resume operations on the prior data - this is however dependant on
+	// the behaviour of the rootstore provided on database instance creation, as this function will Close
+	// the provided rootstore.


nitpick: idk if we care lol but I realize this is just American vs UK spelling diffs at this point.

Suggested change

// It does not explicitly clear any data from persisted storage, and a new [DB] instance may typically

// be created after calling this to resume operations on the prior data - this is however dependant on

// the behaviour of the rootstore provided on database instance creation, as this function will Close

// the provided rootstore.

// It does not explicitly clear any data from persisted storage, and a new [DB] instance may typically

// be created after calling this to resume operations on the prior data - this is however dependent on

// the behavior of the rootstore provided on database instance creation, as this function will Close

// the provided rootstore.

https://ruggedthuglife.com/canada/how-do-you-spell-behaviour-in-canada/ I'm fairly sure we never agreed to use US English, but I might be wrong.

https://www.merriam-webster.com/words-at-play/spelling-variants-dependent-vs-dependant also has similar issues :)

UK English always. Not sure why Americans need to try to be special with everything...

shahzadlone · 2023-04-06T17:30:53Z

client/db.go

+	// Events returns the database event queue.
+	//
+	// It may be used to monitor database events - a new event will be yielded for each mutation.
+	// Note: it does not copy the queue, just the reference to it.


praise: useful note

shahzadlone · 2023-04-06T17:31:37Z

client/db.go

 	PrintDump(ctx context.Context) error
 }

+// Store contains the core Defra read-write operations.


todo:

Suggested change

// Store contains the core Defra read-write operations.

// Store contains the core DefraDB read-write operations.

client/db.go

fredcarle · 2023-04-06T18:40:12Z

client/db.go

+	// All schema types provided must not exist prior to calling this, and they may not reference existing
+	// types previously defined.


thought(out of scope): Reading this I realize that we should probably allow new schema to reference existing ones. For example, I may want to add an Image schema that references the User collection (images are owned by users).

fredcarle · 2023-04-06T18:46:01Z

client/db.go

+	// CreateCollection creates a new collection using the given description.
+	//
+	// WARNING: It does not currently update the GQL types, and as such a database restart is required after
+	// calling this if use of the new collection via GQL is desired (for example via [ExecRequest]).
 	CreateCollection(context.Context, CollectionDescription) (Collection, error)


though: Seeing this warning makes me think that maybe this function shouldn't be exposed publicly. Unless I'm missing something, we don't use this function anywhere internally. We only use the private version of it for the adding schemas. It feels like giving users a chance to do something they may not want to.

I think it is still nominally useful, and my preference long term is to fix both this and UpdateCollection so that they do actually update the GQL types.

That said, I think the lack of references might be quite new - I think they were referenced until the client txn rework, but I'm not 100% sure.

I have just spotted that UpdateCollection is missing this warning and should have it too

Add warning to UpdateCollection

Should we remove them until they do update the GQL types?

I dont really think so, they will still work (on restart) and leaving them in documented like this should cause no harm IMO.

Can even be fixed in 0.5.1 if we want I think, as it wouldnt quite be a breaking change IMO

I've merged, but happy to remove these if that is what we decide upon

client/db.go

fredcarle · 2023-04-06T18:50:56Z

client/db.go

 	GQL GQLResult
+
+	// Pub contains a pointer to an events stream which channels any subscription results


nitpick: Here I think events should be singular event as a stream implies multiple events coming one at a time. It's not a stream of events block.

Note: I don't have an english major so I may be wrong here but that is my interpretation of it.

cheers - will change

events => event

Note: Upon writing this I do not actually think this function should close the rootstore, as Defra does not own it - it is an input parameter and should only be closed by the thing that created it.

Uses the existing internal documentation as a starting point, expands it and then backports the changes to the internal functions.

Note: I think the naming of this and its type is not ideal.

codecov · 2023-04-06T19:52:36Z

Codecov Report

Merging #1305 (177b952) into develop (055e2ea) will decrease coverage by 0.08%.
The diff coverage is n/a.

❗ Current head 177b952 differs from pull request most recent head a52060f. Consider uploading reports for the commit a52060f to get more accurate results

@@             Coverage Diff             @@
##           develop    #1305      +/-   ##
===========================================
- Coverage    70.47%   70.39%   -0.08%     
===========================================
  Files          184      184              
  Lines        17825    17825              
===========================================
- Hits         12562    12548      -14     
- Misses        4311     4321      +10     
- Partials       952      956       +4

Impacted Files	Coverage Δ
client/value.go	`100.00% <ø> (ø)`
db/txn_db.go	`46.26% <ø> (ø)`

... and 5 files with indirect coverage changes

* Document client.DB * Document client.DB.Store * Document client.DB.NewTxn functions * Document client.DB.Root * Document client.DB.Blockstore * Document client.DB.Close Note: Upon writing this I do not actually think this function should close the rootstore, as Defra does not own it - it is an input parameter and should only be closed by the thing that created it. * Document client.DB.Events * Document client.DB.MaxTxnRetries * Document client.DB.PrintDump * Document client.Store * Document client.Store.AddSchema Uses the existing internal documentation as a starting point, expands it and then backports the changes to the internal functions. * Document client.Store.CreateCollection * Document client.Store.GetCollectionByName * Document client.Store.GetCollectionBySchemaID * Document client.Store.GetCollectionByVersionID * Document client.Store.GetAllCollections * Document client.Store.ExecRequest * Document client.GQLResult * Document client.GQLResult.Errors * Document client.GQLResult.Data * Document client.RequestResult * Document client.RequestResult.GQL Note: I think the naming of this and its type is not ideal. * Document client.RequestResult.Pub * Remove commented out code

AndrewSisley added documentation Improvements or additions to documentation area/api Related to the external API component action/no-benchmark Skips the action that runs the benchmark. labels Apr 6, 2023

AndrewSisley added this to the DefraDB v0.5 milestone Apr 6, 2023

AndrewSisley requested a review from a team April 6, 2023 17:12

AndrewSisley self-assigned this Apr 6, 2023

AndrewSisley changed the title ~~doc: Document client interfaces in client/db.go~~ docs: Document client interfaces in client/db.go Apr 6, 2023

shahzadlone approved these changes Apr 6, 2023

View reviewed changes

AndrewSisley force-pushed the sisley/doc/I1303-client-docs branch from cfe2c8f to 35009e9 Compare April 6, 2023 18:02

fredcarle reviewed Apr 6, 2023

View reviewed changes

AndrewSisley added 20 commits April 6, 2023 15:50

Document client.DB

aed4040

Document client.DB.Store

b6cef50

Document client.DB.NewTxn functions

d3719aa

Document client.DB.Root

b31415f

Document client.DB.Blockstore

73b25a8

Document client.DB.Close

81f633d

Note: Upon writing this I do not actually think this function should close the rootstore, as Defra does not own it - it is an input parameter and should only be closed by the thing that created it.

Document client.DB.Events

3a62c2f

Document client.DB.MaxTxnRetries

c22364c

Document client.DB.PrintDump

818ee77

Document client.Store

0b95bc5

Document client.Store.AddSchema

a8228f4

Uses the existing internal documentation as a starting point, expands it and then backports the changes to the internal functions.

Document client.Store.CreateCollection

4aa0e4d

Document client.Store.GetCollectionByName

e7d6139

Document client.Store.GetCollectionBySchemaID

655650b

Document client.Store.GetCollectionByVersionID

edaa039

Document client.Store.GetAllCollections

43c1fb4

Document client.Store.ExecRequest

c7f689b

Document client.GQLResult

70b1419

Document client.GQLResult.Errors

cd5b0f5

Document client.GQLResult.Data

05e8695

AndrewSisley added 9 commits April 6, 2023 15:50

Document client.RequestResult

961ad62

Document client.RequestResult.GQL

818e175

Note: I think the naming of this and its type is not ideal.

Document client.RequestResult.Pub

63bc34f

Remove commented out code

f44c0e2

PR FIXUP - Fix spelling (programmatic)

16ffc02

PR FIXUP - DefraDB vs Defra

3001b2d

PR FIXUP - Fix spelling (constructed)

428cb45

PR FIXUP - Add warning to UpdateCollection

8fdac05

PR FIXUP - Fix pluralization (event)

a52060f

AndrewSisley force-pushed the sisley/doc/I1303-client-docs branch from 177b952 to a52060f Compare April 6, 2023 19:50

AndrewSisley merged commit b08fe6f into develop Apr 6, 2023

AndrewSisley deleted the sisley/doc/I1303-client-docs branch April 6, 2023 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Document client interfaces in client/db.go #1305

docs: Document client interfaces in client/db.go #1305

AndrewSisley commented Apr 6, 2023 •

edited

Loading

shahzadlone left a comment

shahzadlone Apr 6, 2023

AndrewSisley Apr 6, 2023 •

edited

Loading

shahzadlone Apr 6, 2023

AndrewSisley Apr 6, 2023 •

edited

Loading

shahzadlone Apr 6, 2023

AndrewSisley Apr 6, 2023 •

edited

Loading

shahzadlone Apr 6, 2023

AndrewSisley Apr 6, 2023 •

edited

Loading

shahzadlone Apr 6, 2023

shahzadlone Apr 6, 2023

shahzadlone Apr 6, 2023

AndrewSisley Apr 6, 2023 •

edited

Loading

fredcarle Apr 6, 2023

shahzadlone Apr 6, 2023

shahzadlone Apr 6, 2023

fredcarle Apr 6, 2023

fredcarle Apr 6, 2023

AndrewSisley Apr 6, 2023 •

edited

Loading

fredcarle Apr 6, 2023

AndrewSisley Apr 6, 2023

AndrewSisley Apr 6, 2023

fredcarle Apr 6, 2023

AndrewSisley Apr 6, 2023 •

edited

Loading

codecov bot commented Apr 6, 2023 •

edited

Loading

	// DB is the primary public programatic access point to the local Defra instance.
	// DB is the primary public programmatic access point to the local Defra instance.

	// It should be contructed via the [db] package, via the [db.NewDB] function.
	// It should be constructed via the [db] package, via the [db.NewDB] function.

		type DB interface {
		// Store contains Defra database functions protected by an internal, short-lived, transaction, allowing safe

	// Store contains Defra database functions protected by an internal, short-lived, transaction, allowing safe
	// Store contains DefraDB functions protected by an internal, short-lived, transaction, allowing safe

	// Root returns the underlying root store, within which all data managed by Defra is held.
	// Root returns the underlying root store, within which all data managed by DefraDB is held.

		Root() datastore.RootStore

		// Blockstore returns the blockstore, within which all blocks (commits) managed by Defra are held.

	// Store contains the core Defra read-write operations.
	// Store contains the core DefraDB read-write operations.

		// All schema types provided must not exist prior to calling this, and they may not reference existing
		// types previously defined.

		GQL GQLResult

		// Pub contains a pointer to an events stream which channels any subscription results

docs: Document client interfaces in client/db.go #1305

docs: Document client interfaces in client/db.go #1305

Conversation

AndrewSisley commented Apr 6, 2023 • edited Loading

Relevant issue(s)

Description

shahzadlone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Apr 6, 2023 • edited Loading

Codecov Report

AndrewSisley commented Apr 6, 2023 •

edited

Loading

AndrewSisley Apr 6, 2023 •

edited

Loading

AndrewSisley Apr 6, 2023 •

edited

Loading

AndrewSisley Apr 6, 2023 •

edited

Loading

AndrewSisley Apr 6, 2023 •

edited

Loading

AndrewSisley Apr 6, 2023 •

edited

Loading

AndrewSisley Apr 6, 2023 •

edited

Loading

AndrewSisley Apr 6, 2023 •

edited

Loading

codecov bot commented Apr 6, 2023 •

edited

Loading