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

feat: add support for aggregated data for sitewise data source #6

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

NorbertNader
Copy link
Contributor

Issue #, if available:

Description of changes:
add support for aggregated data for sitewise data source

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

import { DataStreamCallback } from '../../../data-module/types';
import { isDefined } from '../../../common/predicates';

const getAggregatedPropertyDataPointsForProperty = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

why not to use async/await for this function too ?

Copy link
Contributor Author

@NorbertNader NorbertNader Nov 19, 2021

Choose a reason for hiding this comment

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

we don't have to since getAggregatedPropertyDataPointsForProperty returns a promise and getAggregatedPropertyDataPoints resolves the promise with Promise.all

@@ -58,3 +58,38 @@ export const toDataPoint = (assetPropertyValue: AssetPropertyValue | undefined):
y: dataValue,
};
};

// TODO: support outputting multiple sets of DataStream for multiple aggregate types.
const aggregateToValue = ({ average, count, maximum, minimum, sum }: Aggregates): number => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see Aggregates being imported on this file ? eslint did not flag this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, yeah looks like I also missed AggregatedValue

Copy link
Contributor

Choose a reason for hiding this comment

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

There's some work to be done on the build system, I don't think it'll fail yarn build from eslint. For now run yarn fix to check for any errors

@@ -58,3 +58,38 @@ export const toDataPoint = (assetPropertyValue: AssetPropertyValue | undefined):
y: dataValue,
};
};

// TODO: support outputting multiple sets of DataStream for multiple aggregate types.
const aggregateToValue = ({ average, count, maximum, minimum, sum }: Aggregates): number => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's some work to be done on the build system, I don't think it'll fail yarn build from eslint. For now run yarn fix to check for any errors

expect(onError).toBeCalled();
});

it('returns data point on success', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

wed'll need to add some pagination tests - though missing them on the other paginated data calls as well, fine as a follow up item

@NorbertNader NorbertNader merged commit eb1e7f4 into main Nov 19, 2021
@NorbertNader NorbertNader deleted the sitewise-aggreagted-data branch February 22, 2022 17:58
TheEvilDev pushed a commit that referenced this pull request Nov 2, 2022
Co-authored-by: Norbert Nader <nnader@amazon.com>
This was referenced Nov 2, 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.

None yet

3 participants