Skip to content

Stop models converting to camelCase#173

Merged
callumforrester merged 7 commits intomainfrom
no-auto-camelcase-of-plan-parameters
May 24, 2023
Merged

Stop models converting to camelCase#173
callumforrester merged 7 commits intomainfrom
no-auto-camelcase-of-plan-parameters

Conversation

@callumforrester
Copy link
Copy Markdown
Contributor

@callumforrester callumforrester commented May 5, 2023

Previously with #132, all config, API parameter and plan model parameter names were converted to camelCase when loaded externally (e.g. from JSON). So

class TaskResponse(BlueapiBaseModel):
    """
    Acknowledgement that a task has started, includes its ID
    """

    task_name: str = Field(description="Unique identifier for the task")

Would accept the following via the API:

{"taskName": "my task"}

and a plan like

def my_plan(foo_bar: str) -> MsgGenerator:
    ...

Would accept:

{"name": "my_plan", "params": {"fooBar": "qwerty"}}

This PR removes all aliasing and we accept that there are snake_case models

@tpoliaw
Copy link
Copy Markdown
Contributor

tpoliaw commented May 15, 2023

I would be in favour of this. Having the request format be able to take multiple forms just add confusion. eg how would it handle requests for

def demo(foo_bar: int = 42, fooBar: str = "42") -> MsgGenerator:
    pass

It's a slightly contrived example but I don't see the benefit to allowing variations in general.

@callumforrester
Copy link
Copy Markdown
Contributor Author

I think you're right, I was assuming that code formatters would shout at people trying to use camel case arguments, but we can't rely on everyone having a formatter.

@callumforrester callumforrester force-pushed the no-auto-camelcase-of-plan-parameters branch from 412e19d to 648e212 Compare May 18, 2023 08:36
@callumforrester
Copy link
Copy Markdown
Contributor Author

If no one strongly objects I'm going to merge this, will make life easier for artemis purposes

@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2023

Codecov Report

Merging #173 (edb3058) into main (d31287f) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
- Coverage   89.90%   89.87%   -0.03%     
==========================================
  Files          41       41              
  Lines        1248     1245       -3     
==========================================
- Hits         1122     1119       -3     
  Misses        126      126              
Impacted Files Coverage Δ
src/blueapi/utils/base_model.py 100.00% <100.00%> (ø)

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

Copy link
Copy Markdown
Contributor

@tpoliaw tpoliaw left a comment

Choose a reason for hiding this comment

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

Should non-plan models still allow camelCase aliasing? Seems weird if it works in some places but not others.

Comment thread tests/core/test_context.py
Comment thread src/blueapi/utils/base_model.py
@callumforrester callumforrester force-pushed the no-auto-camelcase-of-plan-parameters branch from 14a9392 to 453363d Compare May 23, 2023 12:54
@callumforrester callumforrester changed the title Stop plan parameter models converting to camelCase Stop models converting to camelCase May 23, 2023
@callumforrester callumforrester force-pushed the no-auto-camelcase-of-plan-parameters branch from d528cd3 to b9d0975 Compare May 23, 2023 14:26
Comment thread docs/developer/explanations/decisions/0003-api-case.rst
@callumforrester callumforrester force-pushed the no-auto-camelcase-of-plan-parameters branch from b1094be to 6d18720 Compare May 24, 2023 15:00
@callumforrester callumforrester merged commit 51a4cf2 into main May 24, 2023
@callumforrester callumforrester deleted the no-auto-camelcase-of-plan-parameters branch May 24, 2023 15:11
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.

3 participants