Skip to content

Create distinct types for Settings / SettingsPatch#465

Merged
cguedes merged 14 commits into
mainfrom
settings-api-flat-patch
Sep 1, 2023
Merged

Create distinct types for Settings / SettingsPatch#465
cguedes merged 14 commits into
mainfrom
settings-api-flat-patch

Conversation

@danvk

@danvk danvk commented Aug 31, 2023

Copy link
Copy Markdown
Collaborator

This solves the problem with optional fields in SettingsSchema in TypeScript.

In general I think we should avoid specifying default values in Pydantic models that are part of the HTTP API because the semantics are unclear. If you have something like:

class Settings(RefStudioModel):
    a: str = 'alice'
    b: str = 'bob'

@settings_api.put("/")
async def update_settings(req: SettingsSchema) -> SettingsSchema:
    ...

Then if the users hits the PUT endpoint with {"a": "archer"}, presumably their intention is to just set "a" to "archer" and not also set "b" to "bob".

Highlights:

  • I "flattened" the settings schema like we talked about last week. The new model is FlatSettingsSchema and it has no Optional fields or fields with default values.
  • I found this decorator to be the best way to make an all-optional version of a Pydantic model. This is like Partial<T> in TypeScript. I used this to define a FlatSettingsSchemaPatch model. This might be a useful pattern going forward. But if the update type is something more complicated (say you want to drop an id field that can't be updated) then maybe just writing out the two models explicitly is the way to go.
  • Since the settings.json format has changed, I wrote a migration function.
  • With a flat settings structure, the getDotNotation from tauri-settings isn't useful anymore and we can replace it with something much simpler. In fact, we can get rid of tauri-settings altogether! 🎉

@codecov

codecov Bot commented Aug 31, 2023

Copy link
Copy Markdown

Codecov Report

Merging #465 (4fece52) into main (255a870) will decrease coverage by 0.02%.
The diff coverage is 82.89%.

@@            Coverage Diff             @@
##             main     #465      +/-   ##
==========================================
- Coverage   80.50%   80.48%   -0.02%     
==========================================
  Files         187      188       +1     
  Lines       11006    11087      +81     
  Branches     1074     1076       +2     
==========================================
+ Hits         8860     8923      +63     
- Misses       2132     2150      +18     
  Partials       14       14              
Files Changed Coverage Δ
src/application/listeners/projectEventListeners.ts 37.50% <0.00%> (ø)
src/api/typed-api.ts 91.45% <44.44%> (-8.55%) ⬇️
src/settings/settingsManager.ts 37.14% <52.17%> (-8.81%) ⬇️
python/sidecar/pydantic_utils.py 85.00% <85.00%> (ø)
python/sidecar/settings.py 96.15% <92.85%> (-1.63%) ⬇️
python/sidecar/chat.py 94.02% <100.00%> (ø)
python/sidecar/http.py 81.56% <100.00%> (ø)
python/sidecar/rewrite.py 94.44% <100.00%> (ø)
python/sidecar/typing.py 97.76% <100.00%> (+0.16%) ⬆️
src/AppStartup.tsx 90.62% <100.00%> (ø)
... and 4 more

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

@danvk danvk marked this pull request as ready for review August 31, 2023 21:50
return filepath


def default_settings() -> FlatSettingsSchema:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a way to define default settings without adding default values to the type.

Comment thread python/sidecar/typing.py
openai_temperature: float


@make_optional()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is how you make an optional model with the decorator

Comment thread src/api/api-paths.ts
status: ResponseStatus;
};
/** FlatSettingsSchema */
FlatSettingsSchema: {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The new types are much nicer -- all fields required!

}

// TODO: remove this after we separate the request/response types in the settings API
type DeepRequired<T> = T extends object ? { [k in keyof T]-?: DeepRequired<T[k]> } : T;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

✂️ 🍾 🎉

let settings: SettingsSchema;
let settings: FlatSettingsSchema;
try {
settings = (await apiGetJson('/api/settings/')) as SettingsSchema;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

no more type assertion! 🎉

@danvk danvk requested review from cguedes and gjreda August 31, 2023 21:55

@cguedes cguedes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Only added a note about the migrate_settings utility

Comment thread package.json
"react-popper": "^2.3.0",
"react-resizable-panels": "^0.0.51",
"tailwindcss": "^3.3.2",
"tauri-settings": "^0.3.2",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👏 🎉

Comment thread python/sidecar/settings.py
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