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

Allow additional properties in DashboardParameter #60

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

michal-billtech
Copy link
Contributor

📝 Description of the PR

Adds support for additional properties in DashboardParameter

@flovouin
Copy link
Owner

Hi @michal-billtech, thanks for this and sorry for the delay.
Do you expect to use more properties than what you included in the test?
If not, how about adding them as their own properties in the OpenAPI?

@michal-billtech
Copy link
Contributor Author

I saw that you took a similar approach with CardParameter. It seemed to me more beneficial since there seems to be no schema validation messages passed to the user and thus no real benefit to explicitly defining the scheme. On the other hand doing it this way makes it less prone to schema updates on Metabase's side. Am I missing something? Or maybe you've plans to implement schema validation reporting?

@flovouin
Copy link
Owner

Thanks for the reply. To be fair I didn't remember this was a pattern I had already used. Let's merge it like this.

@flovouin flovouin merged commit e3801f0 into flovouin:main Jun 27, 2024
1 check passed
@flovouin flovouin mentioned this pull request Jun 27, 2024
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