FIX: ensure we dont collapse data multiple times (PR #13399)

Note that this commit will also disable daily grouping for datasets with more than 30 data points. This will also smartly do the grouping by month when grouping a full year.

GitHub

This pull request introduces 1 alert when merging 36740bbed2b17a5906574e710444767647855470 into b0416cb1c11ba762da63ebdb297dbd95da334df9 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable
    const options = ["weekly", "monthly"];

In general – a real good extracting of logic, it makes things clearer :+1:.

It’s a bit confusing that this function takes grouping as an argument and at the same time calculates grouping value depending on data.

Probably the intention was to let a caller disable automatic grouping resolution by passing explicit grouping value? If so, it looks like this code would overwrite the grouping argument anyway. Probably this should be fixed.

I would go even further though, but I’m not insisting on this. I feel like this function is trying to be too smart, this implementation can be considered as a violation of the single responsibility design principle. This function

  • chooses an appropriate grouping
  • collapses data using this grouping

The first task can be moved into a separate function with an appropriate name which will make the code clearer.

        const date = moment(d.x, "YYYY-MM-DD");

This seems to share some code with _unitForDatapoints above. Could it be DRYed up?

It mostly share the constants, so yes maybe I could use constants

LGTM :+1: