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

Multiple datasets support #46

Merged
merged 41 commits into from
Jul 16, 2020
Merged

Multiple datasets support #46

merged 41 commits into from
Jul 16, 2020

Conversation

jcc37-zz
Copy link
Collaborator

@jcc37-zz jcc37-zz commented Jun 30, 2020

Branched off of #44

  • Add dataset types and interfaces to expose general APIs to retrieve data from different sources
  • Refactor PreferenceSerice and support "select" type preference item
  • Modify DataService and Preference to support switching between different sets of dataset preference
  • Merge Add tests for d3 #26 to Add tests for services #30 and update old tests for the new code structure and data flow
  • Add new tests for component and dataset

What's next

  • Refactor UserWhiteNoiseDataset
    • Create util functions to create dataset query function with lazy-query and caches
    • Move common category generator to utils, like nthDay in DataCube
  • Refactor XYChartD3
    • Support data points with different x and y types for other chart components

@jcc37-zz jcc37-zz requested a review from 64json June 30, 2020 08:53
@jcc37-zz jcc37-zz marked this pull request as draft June 30, 2020 18:16
src/datasets/types.ts Outdated Show resolved Hide resolved
src/datasets/types.ts Outdated Show resolved Hide resolved
src/datasets/types.ts Outdated Show resolved Hide resolved
src/d3/xy-chart.d3.ts Outdated Show resolved Hide resolved
src/datasets/types.ts Outdated Show resolved Hide resolved
src/datasets/types.ts Outdated Show resolved Hide resolved
src/datasets/types.ts Outdated Show resolved Hide resolved
@jcc37-zz jcc37-zz marked this pull request as ready for review July 7, 2020 04:13
@64json 64json changed the base branch from material to master July 9, 2020 06:30
@jcc37-zz jcc37-zz requested a review from anumeha16 as a code owner July 9, 2020 20:53
@jcc37-zz jcc37-zz mentioned this pull request Jul 10, 2020
src/datasets/metas/types.ts Outdated Show resolved Hide resolved
src/components/card/card.component.ts Outdated Show resolved Hide resolved
src/services/preference/preference.service.ts Show resolved Hide resolved
},
};

export function create(config: Config): Dataset {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to do this in this PR, but generally it's nice to have a small documentation for exported methods, services or any significant chunk of code so that someone can get the gist of it without reading through all the code

Copy link
Collaborator Author

@jcc37-zz jcc37-zz Jul 15, 2020

Choose a reason for hiding this comment

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

It raises another design question here. Actually Config, configMeta, and create are three necessary exports/properties for all dataset module/object, it is nice to have a abstract parent interface or class for all datasets to extend. However, they would be all static and immutable properties and I don't want developers to create instance of dataset if I make them as classes, so I make them modules when adding dataset support.
The bad things of this design are that it is hard to build mock datasets for testing and do the type inference on the map of dataset modules:

export const datasets = {

I would like to refactor it if you have any good ideas.

Copy link
Collaborator

@64json 64json Jul 15, 2020

Choose a reason for hiding this comment

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

Off-topic but it would be great if TypeScript supports a module to implement an interface:

export module implements AbstractDataset;

Then all the members defined has to be implemented and exported, so people don't need to have an extra wrapper to make it type-safe.

EDIT: found similar discussions on microsoft/TypeScript#420 and microsoft/TypeScript#38511.

@jcc37-zz jcc37-zz requested a review from nattySP as a code owner July 15, 2020 21:22
@jcc37-zz jcc37-zz merged commit fc52bc7 into master Jul 16, 2020
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.

None yet

5 participants