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

Should it be possible to set anything as a display type in Store.insert()? #130

Closed
augustebaum opened this issue Aug 1, 2024 · 4 comments
Labels
question Further information is requested

Comments

@augustebaum
Copy link
Contributor

Currently in Store.insert() we perform no check that the input display type is a proper DisplayType. This is because the behaviour when this happens was not specified.

The current behaviour enables a use-case of a user defining their own type, but it conflicts with our approach to strongly type everything in order to extract intelligence reliably.

@tuscland
Copy link
Member

tuscland commented Aug 9, 2024

I understand that this means a DisplayType registry is missing: if we give users the ability to define their own, the registration should be made explicitly, so that values provided as the display_type argument are consistently checked.

@thomass-dev
Copy link
Collaborator

thomass-dev commented Aug 9, 2024

This is not exactly the subject of this issue. We should have clarified.

The validity of the value of the parameter display_type is always compared to the enum of DisplayType.
But under the hood, there is one data class by display_type, which is instantiated only at the last moment: during the export data from store to dashboard.

This data class is strongly typed, so if user specified one display_type, but the underlying data are not compliant, exception will be raise.

The question here is : do we need to check earlier that data are compliant to display_type, or not.

In my opinion, this is not necessary as it could broke the workflow.
The use of our library shoul be non-blocking. In this particular case, we should accept as many calls as possible, especially because this will only impact the dashboard, not the store itself.
If an error occurs when exporting data from store to dashboard, we should simply re-export data with a raw type accompanied by a warning such as wrong display type.

@tuscland
Copy link
Member

tuscland commented Aug 9, 2024

Got it. Yes I agree with your conclusion.

@tuscland
Copy link
Member

Closed by #303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants