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

NumberFormatter architecture is flawed #9061

Closed
diosmosis opened this issue Oct 20, 2015 · 3 comments
Closed

NumberFormatter architecture is flawed #9061

diosmosis opened this issue Oct 20, 2015 · 3 comments
Labels
answered For when a question was asked and we referred to forum or answered it. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.

Comments

@diosmosis
Copy link
Member

The current NumberFormatter class is not built into the existing metrics formatting system in core. Instead, it essentially re-formats values, trying to guess the meaning of a value, after the meaning is removed or transformed. This approach is prone to error and adds an unnecessary extra layer of logic.

Instead, the logic in NumberFormatter should be included in the Html MetricsFormatter (if it is only for UI), or included in the MetricsFormatter base. If neither of these work for some reason, a new base class can be used. But the MetricsFormatter base should be the only way to format values for display and no method should try to guess how to format a value. Users should explicitly specify how they want values to be formatted.

This technical debt should be removed before the next major release.

@diosmosis diosmosis added the c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. label Oct 20, 2015
@diosmosis diosmosis added this to the 3.0.0 milestone Oct 20, 2015
@sgiehl
Copy link
Member

sgiehl commented Oct 20, 2015

👍
the reason I have build it that way, is that it shouldn't affect API responses for 2.15
For 3.0 we should move all formatting to the MetricsFormatter and remove it from the frontend.
Also we should mark all numeric, percentage and money columns, so they can automatically be formatted or something like that.

@diosmosis
Copy link
Member Author

Also we should mark all numeric, percentage and money columns, so they can automatically be formatted or something like that.

For processed metrics, this is done by each ProcessedMetric subclass (the format() method is passed a a MetricsFormatter). Aggregated metrics aren't handled this way at the moment, but they should be.

@mattab mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Oct 20, 2015
@mattab
Copy link
Member

mattab commented Jul 8, 2016

this will get done over time 👍 but we probably don't need to keep track of this tech debt as issue

@mattab mattab closed this as completed Jul 8, 2016
@mattab mattab added answered For when a question was asked and we referred to forum or answered it. and removed Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. labels Jul 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered For when a question was asked and we referred to forum or answered it. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.
Projects
None yet
Development

No branches or pull requests

3 participants