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

Number formatting may be wrong when eg German language is used #16295

Merged
merged 2 commits into from Aug 17, 2020

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Aug 14, 2020

In custom reports. fix dev-1973.

Ran the tests in custom reports and seems to work fine

@tsteur tsteur added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Aug 14, 2020
@tsteur tsteur added this to the 3.14.1 milestone Aug 14, 2020
@tsteur tsteur added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Aug 14, 2020
@sgiehl
Copy link
Member

sgiehl commented Aug 14, 2020

Should we maybe create a new issue to refactor that whole behavior in a later release?
I guess the problem is, that custom reports use metric classes for all available metrics, while all core reports use string arrays as metrics (except of processed ones). Imho it would make sense to always use metric classes for all reports. That way we would have the same behavior everywhere and could do the formatting for all metrics at the same place maybe? 🤔

@tsteur
Copy link
Member Author

tsteur commented Aug 14, 2020

It's actually not so much the problem that metric classes were used but that custom reports basically returned processedMetrics for metrics that weren't processed. It's of course still kind of doing it partially and it's not ideal but reckon for now don't need to create an issue or change anything further. Ideally getProcessedMetrics and metricsToFormat be separated eventually.

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

worked locally

@tsteur tsteur merged commit 62a0920 into 3.x-dev Aug 17, 2020
@tsteur tsteur deleted the customprocessreport branch August 17, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants