Feature/awf/cleanup time agg param#212 #231

CloudNiner · 2017-08-31T20:08:59Z

Overview

Replaces the string type of the time_aggregation indicator param with a new enum type TimeAggParam. This allows for improved type checking.

Assigned both of you so you can see this change. @flibbertigibbet because I mentioned I was making it in a previous PR you authored, and @fungjj92 so it can inform your work on the percentile indicator params.

Demo

No user facing changes

Notes

To make this easier, I replaced the subclassing of IndicatoryQueryOpts by making IndicatorQueryOpts.params have a new IndicatorParams interface that can be extended instead of the QueryOpts interfaces. This provides a much improved type interface for these, and allows us to better type check the params as they're passed around the app, especially when we construct requests to IndicatorService and when we pass the params around via EventEmitters.

The models folder is getting pretty big, and its now very unwieldy to import one item from each file everywhere. I considered adding an index.ts to the models folder and exporting all of the items in that folder there and updating import statements across the app, but that would have cluttered the intent of this PR. Maybe a good separate issue.

Testing Instructions

App should continue to function

Closes #212

Refactor such that IndicatorQueryOpts.params now has type IndicatorParams which can be extended with additional parameters for some indicators. Improves type checking and prevents us from having to write duplicate interface defs for indicators that need to extend the params of IndicatorQueryOpts

Gives these EventEmitters a more specific API contract, to always be of type IndicatorParams, which includes "subtypes" such as ThresholdIndicatorParams

Includes a few more places where we can better typecast params to the new IndicatorParam type

CloudNiner · 2017-09-01T14:11:25Z

Most of below lifted from an offline discussion, but copied here for visibility (@jcahail):

I chose to go with only the yearly time agg option and not provide a user selectable dropdown. The quarterly and monthly options provide too much noise unless a user is zoomed in to a few year's time range, at which point it's not really possible to glean useful information from it.

Monthly option for all years:

Monthly option at ~5yr range:

We could potentially revisit this if we hear a bunch of users requesting it, but for now its less effort and less complicated for users to just omit the option.

flibbertigibbet · 2017-09-01T14:21:23Z

src/app/charts/extra-params-components/threshold.component.ts

@@ -62,11 +62,11 @@ export class ThresholdComponent implements AfterViewInit, OnInit {
    ngAfterViewInit() {
        // Since valueChanges triggers initially before parent is ready, wait until
        // parent is ready here and trigger it to draw chart with extra parameters.
-        this.thresholdParamSelected.emit({data: {
+        this.thresholdParamSelected.emit({


Why is the data object removed here? You had asked for it to be added in another PR.

I think I had misunderstood what the original problem was based on the comments before, combined with forgetting the details of how EventEmitters work. What's here now is the proper way to return data via event emitters, if you do not need the DOM Event

flibbertigibbet · 2017-09-01T14:22:39Z

src/app/models/indicator-params.model.ts

+import { TimeAggParam } from './time-agg-param.enum';
+
+export interface IndicatorParams {
+    climateModels?: ClimateModel[];


Just noticed these are all optional. Shouldn't they be required?

No, because none of these params are required to make an API request, they all have defaults.

flibbertigibbet · 2017-09-01T14:23:37Z

src/app/services/chart.service.ts

@@ -17,7 +17,6 @@ export class ChartService {

    private timeOptions = {
          'yearly': '%Y',
-          'daily': '%Y-%m-%d',


Should there be a quarterly format added here?

Hmm, we don't use it but it wouldn't hurt to be thorough

Actually, can't do this now. It would require additional refactoring. In the quarterly case, we need to convert the keys in the API (e.g. 2010-3) into the start date for a "quarter" which would be July 1, 2010 for 2010-3, and that can't be done with D3.timeParse. We'd need this refactor for pretty much all of the other remaining valid time aggs if we chose to display them in the lab UI.

fungjj92 · 2017-09-01T15:09:28Z

src/app/charts/chart.component.ts

@@ -115,18 +116,15 @@ export class ChartComponent implements OnChanges, OnDestroy, AfterViewInit {
        this.cancelDataRequest();
    }

-    updateChart(extraParams: any) {
+    updateChart(extraParams: IndicatorParams) {
        this.cancelDataRequest();
        this.chartData = [];
        this.rawChartData = [];



I'm not understanding why the typecheck doesn't complain when ThresholdIndicatorParams are sent through

Because interface ThresholdIndicatorParams extends interface IndicatorParams

Ah interesting. Extending works the opposite direction I was expecting, then. Cool.

fungjj92 · 2017-09-01T15:10:31Z

src/app/models/indicator-params.model.ts

@@ -0,0 +1,10 @@
+import { ClimateModel } from './climate-model.model';


Now there are two models:
indicator-parameters.model.ts
indicator-params.model.ts

in the models folder, and that is confusing

Any suggestions for better names? otherwise I'll try to come up with something

We've been using the term "extra params"
Maybe
indicator-extra-params?

Or perhaps indicator-query-parameters

We already have indicator-query-opts and these params aren't always the subset that are the extra params. They're really the GET params to an API request. What about the following refactor:

IndicatorQueryOpts -> IndicatorRequestOpts

IndicatorParams -> IndicatorGetParams

Leave IndicatorParam unchanged

OK IndicatorRequestOpts
2 -- > IndicatorParamsList? or IndicatorQueryStringParams?
OK IndicatorParam

IndicatorQueryParams would just be a shorter version of your second option that isn't any less descriptive. How about that? Definitely like your second suggestion, which is more properly named than even IndicatorGetParams, than the first.

To hopefully reduce confusion: - IndicatorQueryOpts -> IndicatorRequestOpts - IndicatorParams -> IndicatorQueryParams - No change to IndicatorParam

CloudNiner added 3 commits August 31, 2017 15:03

Refactor ExtraParams event emitters

d65bc42

Gives these EventEmitters a more specific API contract, to always be of type IndicatorParams, which includes "subtypes" such as ThresholdIndicatorParams

Refactor to time_aggregation: TimeAggParam

b0a45a1

Includes a few more places where we can better typecast params to the new IndicatorParam type

CloudNiner assigned flibbertigibbet and fungjj92 Aug 31, 2017

CloudNiner requested review from flibbertigibbet and fungjj92 August 31, 2017 20:08

CloudNiner added the in review label Aug 31, 2017

flibbertigibbet approved these changes Sep 1, 2017

View reviewed changes

fungjj92 reviewed Sep 1, 2017

View reviewed changes

CloudNiner mentioned this pull request Sep 1, 2017

Refactor ChartService.convertChartData time formatting to allow handling of other time aggregations #236

Open

Refactor Indicator related model names

64ecb69

To hopefully reduce confusion: - IndicatorQueryOpts -> IndicatorRequestOpts - IndicatorParams -> IndicatorQueryParams - No change to IndicatorParam

CloudNiner merged commit 544ee98 into develop Sep 1, 2017

hectcastro removed the in review label Sep 1, 2017

CloudNiner deleted the feature/awf/cleanup-time-agg-param#212 branch September 1, 2017 16:00

fungjj92 added a commit that referenced this pull request Sep 1, 2017

Clean up after model refactor rebase, PR #231

a3ea761

fungjj92 added a commit that referenced this pull request Sep 1, 2017

Clean up after model refactor rebase, PR #231

1eea740

fungjj92 added a commit that referenced this pull request Sep 1, 2017

Clean up after model refactor rebase, PR #231

b64bf13

fungjj92 added a commit that referenced this pull request Sep 1, 2017

Clean up after model refactor rebase, PR #231

8f993e1

fungjj92 added a commit that referenced this pull request Sep 1, 2017

Clean up after model refactor rebase, PR #231

ba250d6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/awf/cleanup time agg param#212 #231

Feature/awf/cleanup time agg param#212 #231

CloudNiner commented Aug 31, 2017

CloudNiner commented Sep 1, 2017

flibbertigibbet Sep 1, 2017

CloudNiner Sep 1, 2017

flibbertigibbet Sep 1, 2017

CloudNiner Sep 1, 2017

flibbertigibbet Sep 1, 2017

CloudNiner Sep 1, 2017

CloudNiner Sep 1, 2017

fungjj92 Sep 1, 2017

CloudNiner Sep 1, 2017 •

edited

Loading

fungjj92 Sep 1, 2017

fungjj92 Sep 1, 2017

CloudNiner Sep 1, 2017

fungjj92 Sep 1, 2017

fungjj92 Sep 1, 2017

CloudNiner Sep 1, 2017

fungjj92 Sep 1, 2017 •

edited

Loading

CloudNiner Sep 1, 2017 •

edited

Loading

		@@ -0,0 +1,10 @@
		import { ClimateModel } from './climate-model.model';

Feature/awf/cleanup time agg param#212 #231

Feature/awf/cleanup time agg param#212 #231

Conversation

CloudNiner commented Aug 31, 2017

Overview

Demo

Notes

Testing Instructions

CloudNiner commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CloudNiner Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fungjj92 Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

CloudNiner Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

CloudNiner Sep 1, 2017 •

edited

Loading

fungjj92 Sep 1, 2017 •

edited

Loading

CloudNiner Sep 1, 2017 •

edited

Loading