thunderbird / thunderbird-notifications

In-App notifications for Thunderbird
Mozilla Public License 2.0
0 stars 0 forks source link

Features/4 yaml to json pipeline #5

Closed radishmouse closed 3 months ago

radishmouse commented 3 months ago

Closes #4

This PR:

radishmouse commented 3 months ago

@MelissaAutumn I might have gone a little too far for this PR, but I created the NotificationSchema using pydantic and included type and value restrictions.

I'm wondering: would this change (using pydantic to validate the incoming yaml and produce the json) basically close #1 and #3?

Not that we shouldn't do a final check of all of the JSON produced, but what do we check it against? If we have a separate JSON schema file, there are now two sources of truth for what is/isn't valid data. It seems like it would be best if there were only a single source of truth. (And the pydantic model seems like the more robust of the two.)

Please let me know if I'm way off base here! (Also would love to hear your thoughts @malini)

MelissaAutumn commented 3 months ago

I'll look at this more in-depth tomorrow, but I believe you can generate a json schema file from a pydantic model https://docs.pydantic.dev/latest/concepts/json_schema/

Dump a copy, and check to see if it differs in any significant way from our current schema.

We can have a github action later on to auto-generate this per model change. (Manual is fine for now.)

radishmouse commented 3 months ago

I'll look at this more in-depth tomorrow, but I believe you can generate a json schema file from a pydantic model https://docs.pydantic.dev/latest/concepts/json_schema/

Dump a copy, and check to see if it differs in any significant way from our current schema.

We can have a github action later on to auto-generate this per model change. (Manual is fine for now.)

Looking at them side by side, the generated one is pretty close to the one submitted for #1. The only thing not included in the generated schema is the "if CTA exists, URL must exist" requirement.

Also, the generated schema doesn't define min/max constraints for "percent_chance", so I'll add a @field_validator to account for that.

radishmouse commented 3 months ago

@MelissaAutumn I've made a few more changes:

radishmouse commented 3 months ago

@MelissaAutumn really awesome advice! I think it's ready for another pass.