labthings / python-labthings

Python implementation of LabThings, based on the Flask microframework
GNU General Public License v3.0
18 stars 2 forks source link

More openapi improvements #249

Closed rwb27 closed 3 years ago

rwb27 commented 3 years ago

The OpenAPI docs (which power the SwaggerUI among other things) were not valid OpenAPI JSON. They were also missing most of the important docstrings. This PR goes a long way to fixing that.

Things I've improved include:

Known issues:

codecov[bot] commented 3 years ago

Codecov Report

Merging #249 (54d5df9) into master (247920e) will increase coverage by 0.38%. The diff coverage is 99.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
+ Coverage   90.43%   90.82%   +0.38%     
==========================================
  Files          40       41       +1     
  Lines        1746     1831      +85     
  Branches      277      288      +11     
==========================================
+ Hits         1579     1663      +84     
+ Misses        110      108       -2     
- Partials       57       60       +3     
Flag Coverage Δ
unittests 90.82% <99.18%> (+0.38%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/labthings/apispec/utilities.py 96.77% <96.77%> (ø)
src/labthings/apispec/plugins.py 93.02% <100.00%> (+3.68%) :arrow_up:
src/labthings/default_views/actions.py 100.00% <100.00%> (ø)
src/labthings/default_views/docs/__init__.py 100.00% <100.00%> (ø)
src/labthings/default_views/extensions.py 100.00% <100.00%> (ø)
src/labthings/default_views/root.py 88.88% <100.00%> (+8.88%) :arrow_up:
src/labthings/extensions.py 85.07% <100.00%> (ø)
src/labthings/labthing.py 96.70% <100.00%> (ø)
src/labthings/marshalling/args.py 94.44% <100.00%> (ø)
src/labthings/marshalling/marshalling.py 58.82% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 247920e...54d5df9. Read the comment docs.

jtc42 commented 3 years ago

@rwb27 feel free to merge this whenever you like, now all checks are passing.

rwb27 commented 3 years ago

@jtc42 I'm going to merge this - pretty much all I've done since you were last happy to merge is tighten up the unit testing, and fix some minor bugs that uncovered. Now I've increased our coverage 👼

Kaspar and I reviewed the changes (with the caveat that he's not super familiar with the codebase) and identified a few tasks, all of which I've now done:

jtc42 commented 3 years ago

@jtc42 I'm going to merge this - pretty much all I've done since you were last happy to merge is tighten up the unit testing, and fix some minor bugs that uncovered. Now I've increased our coverage 👼

Kaspar and I reviewed the changes (with the caveat that he's not super familiar with the codebase) and identified a few tasks, all of which I've now done:

  • [ x] Add a unit test to make sure two ActionView subclasses with the same name

    • [x] don't break each other

    • [ x] generate a warning (because duplicate names are confusing

  • [x] Check build_action_schema isn't broken for its use in Thing Description (I also added a note to explain that it's no longer needed for OpenAPI)

  • [x] Add a YAML endpoint for openapi docs (and add an "openapi" endpoint as well as the "swagger" one)

Absolutely fine by me! Thanks