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

Normalise prop types of mapped components with ArrayValue #1739

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Jan 23, 2025

From #1738 (comment)

The value, auxValues, errors, auxErrors and axisValues props of mapped components (MappedLineVis, MappedRgbVis, etc.) were typed in a few different, inconsistent ways:

  • with NumArray (= number[] | TypedArray)
  • with ArrayValue<DType> (where DType matches the one of the dataset prop — e.g. Dataset<ArrayShape, NumericType> => ArrayValue<NumericType>, but it works with any other dtype or dtype union like NumericLikeType)
  • with Value<Props['dataset']> to avoid duplicating the dtype.

While the third way is the most robust and DRY, it's not the most readable since it adds an extra level of misdirection. And while the first way is the most explicit, it becomes less readable and/or more error-prone with other dtypes, like NumericLikeType, PrintableType or ComplexType (especially once we start supporting big ints).

So I'm going for the middle ground with ArrayValue.


While I'm at it:

  • I remove NumArrayDataset completely, which was an alias for Dataset<ArrayShape, NumericType>; it adds a couple of imports in a few files, but it's less confusing, especially since we don't have aliases for datasets with other dtypes, like Dataset<ArrayShape, NumericLikeType>.
  • I rename Primitive to ScalarValue. This makes it clearer that ScalarValue is the value of datasets with ScalarShape.

@axelboc axelboc requested a review from loichuder January 23, 2025 16:05
@axelboc axelboc changed the title Normalise mapped components' props with ArrayValue Normalise types of mapped components' props with ArrayValue Jan 23, 2025
@axelboc axelboc changed the title Normalise types of mapped components' props with ArrayValue Normalise prop types of mapped components with ArrayValue Jan 23, 2025
Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

Not bad! Yeah I think it improves things

@axelboc axelboc merged commit 41553e5 into main Jan 24, 2025
8 checks passed
@axelboc axelboc deleted the norm-mapped-props branch January 24, 2025 09:53
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