Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support plain text on enum validated params #10

Merged
merged 5 commits into from
Oct 4, 2024

Conversation

mazen-r
Copy link
Contributor

@mazen-r mazen-r commented Oct 1, 2024

Related issue: #9
Support plain text on params with enum since the config is generated as plain text on the API player.

@mazen-r mazen-r requested a review from Granitosaurus October 1, 2024 09:54
Co-authored-by: Johann Saunier <[email protected]>
@Granitosaurus
Copy link
Contributor

@mazen-r I think we should use literal string values instead of any string to be more idiomatic with typescript. e.g. we should use:

format_options?: ("no_links" | "no_images" | "only_content" | FormatOption)[];
// instead of current:
format_options?: string[] | FormatOption[];

This will prevent users from making mistakes in string options while retaining all type checking.

Tbh I'm not sure what's the use of Enums are these days and string literal types would actually be prefered over Enums every single time except when names change often. Though I don't think we can drop support for Enums in case someone is using this feature already but we can incentivise string use if we put the string type hints before the enum one (like the example above)

So, this PR should be updated to hint string types with literal values where it's appropriate.

@mazen-r
Copy link
Contributor Author

mazen-r commented Oct 3, 2024

@Granitosaurus yeah good point, updated accordingly. Enums were a requirement in the Python SDK since Python doesn't have type-checking capabilities and I followed the same approach here. Also, not all users use TS so string type hints maybe ineffective for such cases.

@Granitosaurus Granitosaurus merged commit cdc4b71 into main Oct 4, 2024
1 check passed
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