Skip to content
This repository was archived by the owner on May 7, 2026. It is now read-only.

Remove unions in params and delete request body#13

Merged
rosesyrett merged 7 commits into
masterfrom
removeUnionsInParamsAndDeleteRequestBody
Aug 18, 2022
Merged

Remove unions in params and delete request body#13
rosesyrett merged 7 commits into
masterfrom
removeUnionsInParamsAndDeleteRequestBody

Conversation

@rosesyrett
Copy link
Copy Markdown
Contributor

This PR addresses two related issues:

  1. Delete requests have a request body (which they shouldn't)
  2. confusing union types in parameters

For the first task, the request body used to have a 'tag_or_idx' parameter which was a union of int or str. Now, I've changed my delete paths to require at least one query parameter, tag or idx. That is /a/path?tag=foo or /a/path?idx=0, which will dictate what is deleted.

Then, I decided to decouple my tag_or_idx parameters in general, mostly in the parameters of request bodies. As a result, my pydantic models for editing orientations/reflections now have separate 'tag' and 'idx' fields.

However, I have one major question about this change - is it best to include these 'tag' and 'idx' fields, which are currently in my EditOrientationParams and EditReflectionParams pydantic models for the request body, to be query parameters? This would be more in line with the new style for deleting orientations/reflections, however to me it also makes more sense for everything to be neatly bundled in the request body where appropriate.

Copy link
Copy Markdown
Contributor

@garryod garryod left a comment

Choose a reason for hiding this comment

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

Overall looks really good, a few little comments on style and readability here and there, but nothing of real significance

I would say they should be part of the query itself as they're the primary thing which is being indexed by, any supplementary data can be included in the body. Though @callumforrester may disagree

Comment thread src/diffcalc_API/routes/hkl.py Outdated
Comment thread src/diffcalc_API/routes/hkl.py Outdated
Comment thread src/diffcalc_API/routes/hkl.py Outdated
Comment thread src/diffcalc_API/routes/hkl.py Outdated
Comment thread src/diffcalc_API/routes/ub.py Outdated
Comment thread src/diffcalc_API/routes/ub.py Outdated
Comment thread src/diffcalc_API/routes/ub.py Outdated
Comment thread src/diffcalc_API/routes/ub.py Outdated
Comment thread src/diffcalc_API/server.py Outdated
Comment thread src/diffcalc_API/services/hkl.py
@rosesyrett rosesyrett merged commit 454ec9f into master Aug 18, 2022
@rosesyrett rosesyrett deleted the removeUnionsInParamsAndDeleteRequestBody branch August 18, 2022 18:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants