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

Standardise api #222

Merged
merged 8 commits into from
May 23, 2023
Merged

Standardise api #222

merged 8 commits into from
May 23, 2023

Conversation

keithralphs
Copy link
Contributor

Rework the existing endpoints to comply with the guidelines:

  • Making url object elements plural
  • Changing PUT to POST for task creation
  • Returning 201 for successful task creation
  • Returning Location header of created resource on successful creation
  • Adding version header to all endpoint responses

API version is set in app declaration after loading it from REST config object.
Added function to allow easy appending of endpoint specific response headers to defaults
Submit task method refactored to return task id (currently UUID)

Copy link
Collaborator

@callumforrester callumforrester 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 this, just a couple of minor points. Also the CI is unhappy.

src/blueapi/config.py Outdated Show resolved Hide resolved
src/blueapi/service/main.py Outdated Show resolved Hide resolved
src/blueapi/service/main.py Outdated Show resolved Hide resolved
@keithralphs
Copy link
Contributor Author

This PR is awaiting Callum's #202, which it will need to be rebased on top of. Until then the Liniting and tests will fail as the signature and purpose of the worker.submit_tasks method and its tests are in flux. Consequently do not merge this until #202 has been cleared and the rebase updates performed

@keithralphs keithralphs marked this pull request as draft May 22, 2023 14:38
)


@app.get("/plans", response_model=PlanResponse)
def get_plans(handler: Handler = Depends(get_handler)):
def get_plans(response: Response, handler: Handler = Depends(get_handler)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

response is no longer used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #222 (d8ab4ee) into main (3b4ab8c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #222      +/-   ##
==========================================
+ Coverage   90.32%   90.37%   +0.04%     
==========================================
  Files          40       40              
  Lines        1168     1174       +6     
==========================================
+ Hits         1055     1061       +6     
  Misses        113      113              
Impacted Files Coverage Δ
src/blueapi/service/main.py 84.00% <100.00%> (+2.18%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@keithralphs keithralphs marked this pull request as ready for review May 23, 2023 15:46
@keithralphs keithralphs merged commit f30aed0 into main May 23, 2023
@callumforrester callumforrester deleted the standardise_api branch May 24, 2023 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants