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

Refactor get method implementation #48

Closed
2 tasks done
surchs opened this issue Jan 4, 2023 · 2 comments · Fixed by #195
Closed
2 tasks done

Refactor get method implementation #48

surchs opened this issue Jan 4, 2023 · 2 comments · Fixed by #195
Assignees
Labels
maint:coverage Test coverage improvements that were not included in feature prioritization type:maintenance Upkeeping efforts & catch-up corrective improvements that are not Features nor Bugs

Comments

@surchs
Copy link
Contributor

surchs commented Jan 4, 2023

I think we could probably rethink the way the get method works overall. I think right now it does too many things:

  1. Act as a wrapper to the create_query method by passing along all arguments intended for that guy
  2. Act as a wrapper for the httpx.post call by handling the header stuff
  3. as an error handler for the post call

We should:

  • get rid of get, put all the logic inside the query router
  • call the query generator outside of get/post, store the output and pass that along
  • make a slimmed down version of a post wrapper that receives the completed query rather than the whole argument chain
  • refactor creation of the SPARQL query context into its own function
@surchs surchs added flag:discuss Flag issue that needs to be discussed before it can be implemented. type:maintenance Upkeeping efforts & catch-up corrective improvements that are not Features nor Bugs maint:coverage Test coverage improvements that were not included in feature prioritization labels Jan 4, 2023
@rmanaem rmanaem self-assigned this Jan 5, 2023
@alyssadai alyssadai self-assigned this Jan 9, 2023
@alyssadai
Copy link
Contributor

This is a fair point - thanks @surchs! A couple initial thoughts from me:

I think the fact that the get method does 2. and 3. is not too far-fetched - I've seen in tutorials examples where something similar is done, but with the get method as a wrapper for a call to a SQL database. I do see that the lack of tests for the create_query function is not ideal, as I suppose we don't check explicitly that the generated query is well-formed aside from Stardog not complaining at us when it's sent through - since we don't have the help of a SPARQL engine to format queries.

In terms of your ideas for simplifying get, I'm leaning more towards either the second or third option since I believe having the CRUD operations separated from the path operations (i.e., @router.get(...)/@app.get(...)) is generally recommended, especially for the purpose of unit tests in more complex apps (see https://fastapi.tiangolo.com/tutorial/sql-databases/#crud-utils and the Tip under the "Read data" code snippet).

I do think we could also (?) have a unit test for the create_query function - I could see this being stored in a separate test module (test_commands.py?), along with any future tests for Stardog post requests (e.g., a second template containing INSERT statements for updating the graph).

@github-actions
Copy link

We want to keep our issues up to date and active. This issue hasn't seen any activity in the last 30 days.
We have applied the stale-issue label to indicate that this issue should be reviewed again and then either prioritized or closed.

@github-actions github-actions bot added the _flag:stale [BOT ONLY] Flag issue that hasn't been updated in a while and needs to be triaged again label Aug 19, 2023
@alyssadai alyssadai changed the title Rethink get method implementation Refactor get method implementation Oct 3, 2023
@alyssadai alyssadai self-assigned this Oct 3, 2023
@alyssadai alyssadai removed _flag:stale [BOT ONLY] Flag issue that hasn't been updated in a while and needs to be triaged again flag:discuss Flag issue that needs to be discussed before it can be implemented. labels Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maint:coverage Test coverage improvements that were not included in feature prioritization type:maintenance Upkeeping efforts & catch-up corrective improvements that are not Features nor Bugs
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants