Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Conversation

obi1kenobi
Copy link
Contributor

@MichaelaShtilmanMinkin I'd appreciate your 👀 on this since you already helped write much of the existing documentation.

@coveralls
Copy link

coveralls commented Oct 24, 2017

Coverage Status

Coverage remained the same at 94.267% when pulling c091e8c on degree_filtering_docs into e32336b on edge_degree_filtering.

@jssandh2
Copy link

Looks good to me, but I'm not the doc expert. Will wait on @MichaelaShtilmanMinkin.

{
Animal {
name @output(out_name: "animal_name")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for the newline here? I don't see it anywhere else in the docs under a similar context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not necessary, it just visually separates the output from the meat of the query with the complicated filter. Would you prefer it removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mostly care about consistency throughout the docs. It is a pretty complicated filter so maybe it's good to keep it.

name @output(out_name: "animal_name")
out_Animal_ParentOf @filter(op_name: "has_edge_degree", value: ["$child_count"]) @optional {
uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we just have uuid here?

Copy link
Contributor Author

@obi1kenobi obi1kenobi Oct 25, 2017

Choose a reason for hiding this comment

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

Because we have to have something between the curly braces, to satisfy the GraphQL parser. The uuid has no effect, since it's not tagged, output, or filtered on.

Should I add an explanation for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please! It seems a little out of place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll add a note. Thanks!

@coveralls
Copy link

coveralls commented Oct 25, 2017

Coverage Status

Coverage remained the same at 94.267% when pulling a152ebb on degree_filtering_docs into e32336b on edge_degree_filtering.

@obi1kenobi obi1kenobi merged commit e9fee81 into edge_degree_filtering Oct 25, 2017
@obi1kenobi obi1kenobi deleted the degree_filtering_docs branch October 25, 2017 15:54
obi1kenobi added a commit that referenced this pull request Oct 25, 2017
* Add UnaryTransformation Expression type, and Literal int values. (#47)

* Add UnaryTransformation Expression type.

* Improve exception language in operator type check.

* Allow @filter directives that apply to the enclosing scope. (#48)

* Allow @filter directives that apply to the enclosing scope.

An example of such a filtering operator is the upcoming edge degree operator.

* Ignore filters that apply to the outer scope at the root of the AST.

* Update filter handler functions to use FilterOperationInfo objects. (#49)

* Implement the new "has_edge_degree" filter. (#50)

* Add docs for the new "has_edge_degree" filter operation. (#51)

* Add docs for the new "has_edge_degree" filter operation.

* Add a note regarding the unused `uuid` field.

* Update the @optional + has_edge_degree docs for clarity.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants