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

Conversation

waxlamp
Copy link
Contributor

@waxlamp waxlamp commented Aug 22, 2019

This works with the client application, but needs some serious review, cleanup, and in-place updates.

Some TODOs:

  • Overhaul the form of all endpoints, and their return types, to be as consistent as possible
  • Move as much as possible from db.py directly into the view functions in api.py
  • Eliminate GraphQL support code from db.py (50bb632)
  • Try to use more AQL in implementing these endpoints
  • Use better error handling
  • Document the API in the Sphinx docs (see Document REST API #155)
  • Document the API via Swagger (see Add Swagger documentation #154)
  • Write tests (see Write API endpoint tests #156)

Some of these may just become new issues, in which case they'll be considered done for the purposes of the TODO list above.

@AlmightyYakob and @JackWilb, please take a look.

Moots and therefore closes #8.
Closes #127 (via 6160bc0).
Closes #150 (via b4ce5fc).
Closes #157 (via f1daea8).

@waxlamp waxlamp requested review from JackWilb and jjnesbitt August 22, 2019 22:21
@JackWilb
Copy link
Member

db.py contains this line from multinet.types import Row, Entity, EntityType, Cursor but the types.py has been deleted

@jjnesbitt
Copy link
Member

db.py contains this line from multinet.types import Row, Entity, EntityType, Cursor but the types.py has been deleted

I've also noticed this, and the file still uses those types all over. However, none of the functions that use these types seem to be called anywhere. Specifically, the functions (all within db.py):

nodes
edges
fetchRows
countNodes
countEdges
graph_node_types
graph_edge_types
source
target
outgoing
incoming

all use these types but are never called. So if all of the behavior of these functions has been replicated somewhere else, I think we can remove them.

@waxlamp
Copy link
Contributor Author

waxlamp commented Aug 23, 2019

Yeah, one of the things left to do on this branch is to delete all of that dead code, something I'll do in my next pass over db.py.

(Also, in general, if you're commenting on a specific line, it's best to put the comment on that line so that we can have individual threads about these issues.)

These are functions that used to be used by GraphQL resolvers in
carrying out various queries.
@waxlamp waxlamp marked this pull request as ready for review August 26, 2019 21:37
@JackWilb
Copy link
Member

I'm having issues seeing the graphs in the client. Not sure if that's a bug with the client? The api is returning 200

@waxlamp
Copy link
Contributor Author

waxlamp commented Aug 27, 2019

Can you be more specific? Is it the incoming/outgoing links for a specific node, or some other part of the view?

@waxlamp
Copy link
Contributor Author

waxlamp commented Aug 27, 2019

Found one bug that might be the one you mentioned; that uncovered another bug I'm working on now.

@jjnesbitt
Copy link
Member

I think I also found the same bug, which that last commit fixed.

@waxlamp
Copy link
Contributor Author

waxlamp commented Aug 27, 2019

Ok, I can see the nodes and also the node edges now. Thanks for the bug reports!

Copy link
Member

@JackWilb JackWilb left a comment

Choose a reason for hiding this comment

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

Overall this is great. I left a couple minor comments for you. I'm also having some general errors with viewing graphs in the client (the edge tables are missing and the nodes/edges don't show up in the main view area) that we should fix before we merge this in.

jjnesbitt
jjnesbitt previously approved these changes Aug 27, 2019
Copy link
Member

@jjnesbitt jjnesbitt left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@JackWilb JackWilb left a comment

Choose a reason for hiding this comment

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

Just one more little thing 😂

@waxlamp waxlamp requested a review from JackWilb August 27, 2019 15:20
Copy link
Member

@JackWilb JackWilb left a comment

Choose a reason for hiding this comment

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

Found another bug: When I go to the graph and then click a node and change the table parameter of the url, the TableNotFound(table) is giving the error __init__() missing 1 required positional argument: 'table'

@JackWilb
Copy link
Member

The same thing happens for graph. I'm guessing it's an error with how the error is raised/ the init method of the errrors

@waxlamp
Copy link
Contributor Author

waxlamp commented Aug 27, 2019

Good catches; fixed!

Copy link
Member

@JackWilb JackWilb left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for all this work, Roni.

@waxlamp waxlamp merged commit b0b4f2f into master Aug 27, 2019
@waxlamp waxlamp deleted the remove-graphql branch August 27, 2019 15:36
@waxlamp
Copy link
Contributor Author

waxlamp commented Aug 27, 2019

Thank you both for your support on this long-running, earthquake-causing branch!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

3 participants