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

feat: Add descriptions to all system defined GQL stuff #1387

Merged
merged 13 commits into from
Apr 24, 2023

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #796

Description

Adds descriptions to all system defined GQL stuff (fields, args, enum values, etc). Also adds a couple of missing tests that I needed in order to be sure of the behaviour I was documenting.

I'm not terribly fussed about the location of the constant declarations, this will need to be redone once Pavneet and co have decided on how they wish to manage these long-term.

All the descriptions have been added in the last commit only, other commits include misc changes (such as tests).

Tested via Altair:

image

@AndrewSisley AndrewSisley added documentation Improvements or additions to documentation area/query Related to the query component action/no-benchmark Skips the action that runs the benchmark. labels Apr 21, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.5.1 milestone Apr 21, 2023
@AndrewSisley AndrewSisley requested a review from a team April 21, 2023 17:37
@AndrewSisley AndrewSisley self-assigned this Apr 21, 2023
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

Merging #1387 (42c9f0d) into develop (56b8a22) will increase coverage by 0.16%.
The diff coverage is 98.48%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1387      +/-   ##
===========================================
+ Coverage    70.56%   70.73%   +0.16%     
===========================================
  Files          184      184              
  Lines        17838    17875      +37     
===========================================
+ Hits         12588    12644      +56     
+ Misses        4293     4280      -13     
+ Partials       957      951       -6     
Impacted Files Coverage Δ
request/graphql/schema/manager.go 97.34% <ø> (-0.03%) ⬇️
request/graphql/schema/generate.go 84.87% <98.44%> (+0.54%) ⬆️
request/graphql/schema/types/types.go 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

@AndrewSisley AndrewSisley force-pushed the sisley/feat/I796-gql-descr branch 2 times, most recently from f297842 to 877f860 Compare April 21, 2023 17:58
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Just a few todos related to the text in the descriptions.

Thanks for adding the tests related to empty sets.

`
dockeysArgDescription string = `
An optional set of dockeys for this field. Only documents with a dockey
matching an dockey in the given set will be returned. If no documents match,
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: matching *a* dockey.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 24, 2023

Choose a reason for hiding this comment

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

  • a dockey

Comment on lines 103 to 104
An optional filter for this join, if none of the related records do
not meet the filter criteria the host record will still be returned,
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo:if none of the related records meet the filter criteria

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 24, 2023

Choose a reason for hiding this comment

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

  • rm extra words

GroupByArgDescription string = `
An optional set of fields for which to group the contents of this field by.
If this argument is provided, only fields used to group may be rendered in
he immediate child selector. Additional fields may be selected by using
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: in the immediate

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 24, 2023

Choose a reason for hiding this comment

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

  • Add missing letter

Comment on lines 38 to 39
commit composed of the field level commits and the prior composite commit
(in the case of an update).
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: field level commits and, in the case of an update, the prior composite commit

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 24, 2023

Choose a reason for hiding this comment

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

  • Nice suggestion, will do - thanks

to determine the state of the data model at the time of commit.
`
commitDeltaFieldDescription string = `
The CBOR encoded representation of the new value that saved as part of this commit.
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: of the value that is saved

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 24, 2023

Choose a reason for hiding this comment

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

I'm 50-50 on that, but will change

  • rm new

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was mostly the "is" that was missing :)

These are the set of fields supported for grouping by in a commits query.
`
commitsQueryDescription string = `
Returns a set of commits matching any provided criteria, if no arguments are
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: criteria. If no

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 24, 2023

Choose a reason for hiding this comment

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

:) feels a bit like a nitpick lol, but will change 😁

  • .

Copy link
Collaborator

Choose a reason for hiding this comment

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

It just that it feels like 2 sentences so the comma feels like the wrong break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha I agree with you lol, I'm just not sure I'd have flagged this as it is quite minor

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that I might be a bit more picky here because it's publicly displayed text 😅

provided all commits in the system will be returned.
`
latestCommitsQueryDescription string = `
Returns a set of head commits matching any provided criteria, if no arguments are
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: criteria. If no

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 24, 2023

Choose a reason for hiding this comment

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

  • .

values.
`
eqOperatorDescription string = `
The equality operator, if the target matches the value the check will pass.
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: From here to line 205, I would change , if to . If.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer the comma, if you really don't like it I can suggest - instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels like it should be all part of the same sentence. "-" is totally fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Indicate the primary side of a one-to-one relationship.
`
relationDirectiveDescription string = `
Allows the explicit definition of relationship attributes, instead of using the system generated
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: remove comma.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 24, 2023

Choose a reason for hiding this comment

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

  • rm comma

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

Cheers for taking care of these.

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Thanks for doing the changes Andy!

@AndrewSisley AndrewSisley merged commit e20f364 into develop Apr 24, 2023
@AndrewSisley AndrewSisley deleted the sisley/feat/I796-gql-descr branch April 24, 2023 21:33
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…#1387)

* Remove out of date todo

* Remove commented out code

* Remove commented out code

* Remove unused GQL type (Delta)

* Do not overwrite key field declaration

* Add test for empty groupBy

* Add empty order test

* Add limit:0 test

* Add test for update with nonexistantID

* Add test for delete ids, empty set

* Add field description to gql fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/query Related to the query component documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add descriptions to all gql types
3 participants