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

Improve Typing for Aesthetics and Transforms #46

Closed

Conversation

DonIsaac
Copy link
Contributor

  • refactor: move Transform type Aesthetic.ts to types.ts and export it
  • refactor: use Transform type in channel type definitions
  • refactor(aesthetics): make dim method strongly typed
  • refactor(aesthetics): make store and scales strongly typed

Copy link
Collaborator

@bmschmidt bmschmidt left a comment

Choose a reason for hiding this comment

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

Thanks for these fixes.

@@ -126,7 +126,7 @@ export class ReglRenderer extends Renderer {
const { prefs } = this;
const { transform } = this.zoom;
const { aes_to_buffer_num, buffer_num_to_variable, variable_to_buffer_num } = this.allocate_aesthetic_buffers();
console.log(prefs.arrow_table);
// console.log(prefs.arrow_table);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to just kill anything that isn't a console.warn or console.error. I would lint them out of existence except there are few cases that they're actually intentional, alas.

Comment on lines -30 to +36
const scales : { [key : string] : Function } = {
const scales: Record<
Transform,
(range?: Iterable<number>) => ScaleContinuousNumeric<number, number, never> | ScaleIdentity<never>
> = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is nice, glad to see this signature clarified.

Comment on lines +692 to +696
type ConcreteAesthetic =
| X
| Y
| Size
| Jitter_speed
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's up with this formatting, especially the line before the first X? I'm conditioned to think of leading lines as guard rather than an OR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same formatting that OCaml uses - I prefer it for multi-line union types as it keeps everything consistent.

@bmschmidt bmschmidt closed this Aug 29, 2022
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