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

new Formatter<T> interface definition #444

Merged
merged 6 commits into from
Jul 6, 2021
Merged

Conversation

RalphSteinhagen
Copy link
Member

addresses issue #432 and targeted to replace other formatter definitions

@RalphSteinhagen RalphSteinhagen linked an issue Jul 3, 2021 that may be closed by this pull request
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:56 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:56 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:56 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:56 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:56 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:56 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:56 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:56 Inactive
addresses issue #432 and targeted to replace other formatter definitions
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:57 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:57 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 16:57 Inactive
@codecov
Copy link

codecov bot commented Jul 3, 2021

Codecov Report

Merging #444 (39f0477) into master (b3a4372) will increase coverage by 0.14%.
The diff coverage is 61.11%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #444      +/-   ##
============================================
+ Coverage     53.25%   53.40%   +0.14%     
- Complexity     7360     7434      +74     
============================================
  Files           386      388       +2     
  Lines         40679    40840     +161     
  Branches       6553     6586      +33     
============================================
+ Hits          21664    21809     +145     
- Misses        17462    17463       +1     
- Partials       1553     1568      +15     
Impacted Files Coverage Δ
...fx-math/src/main/java/de/gsi/math/DataSetMath.java 27.74% <25.71%> (+0.12%) ⬆️
...ataset/src/main/java/de/gsi/dataset/Formatter.java 73.33% <73.33%> (ø)
...src/main/java/de/gsi/math/MultiDimDataSetMath.java 94.07% <90.90%> (-0.55%) ⬇️
...in/java/de/gsi/dataset/DefaultNumberFormatter.java 92.85% <92.85%> (ø)
...hart/plugins/measurements/DataSetMeasurements.java 75.43% <0.00%> (-0.18%) ⬇️
.../main/java/de/gsi/chart/axes/spi/AbstractAxis.java 76.19% <0.00%> (-0.15%) ⬇️
...ava/de/gsi/chart/plugins/YWatchValueIndicator.java 86.27% <0.00%> (+0.98%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3a4372...39f0477. Read the comment docs.

@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy July 3, 2021 17:01 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 17:08 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 3, 2021 17:08 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy July 3, 2021 17:12 Inactive
Copy link
Contributor

@domenicquirl domenicquirl left a comment

Choose a reason for hiding this comment

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

Aside from the comments, there's just one overarching point I'd like to mention, which is the formatting of data sets. As the implementation is now, DataSetMath methods expect Formatter<Number>s and pass the name of any data set that is a formatting parameter as its formatting argument.

While this produces output that matches the current names, this also takes away the ability to supply a more general formatter that applies to datasets. In particular, if I as a user already have a Formatter<Object> with a case for DataSets, which I use in a place where such a formatter is accepted, I cannot re-use that here.

I understand that representing data sets by their name should be the default behaviour and should "just work" for cases where maybe a user actually writes only a Formatter<Number>. Therefore, I propose to special case DataSet in Formatter#format in the following way:

for (var i = 0; i < arguments.length; i++) {
    if (test.isAssignableFrom(arguments[i].getClass())) {
        formatter.setFormatByArgumentIndex(i, numberFormat);
    } else if (arguments[i] instanceof DataSet) {
		// If the argument is a data set, and the user-supplied 
		// formatting does not handle data sets (with the above case), 
		// default to representing data sets by name (with a 
		// formatter implementation `dataSetNameFormat`)
		formatter.setFormatByArgumentIndex(i, dataSetNameFormat);
    }
}

and accept any Formatter<? extends Object> as the vararg parameter on the methods which is given data set objects as formatting args, but keep the current DefaultNumberFormatter, which is a Formatter<Number>, as the default. What do you think?


Apart from that, this PR adapts everything in DataSetMath. Given that the two classes are very similar, MultiDimDataSetMath should probably be included too?

@RalphSteinhagen
Copy link
Member Author

@domenicquirl yes meant to make it Formatter<? extends Object> missed that ... thanks for pointing out.

@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 5, 2021 08:33 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy July 6, 2021 06:33 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy July 6, 2021 06:33 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 07:13 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 07:13 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 07:18 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 07:18 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy July 6, 2021 07:23 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 10:12 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 10:12 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy July 6, 2021 10:16 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 10:41 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 10:41 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 10:41 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage July 6, 2021 10:41 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy July 6, 2021 10:46 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy July 6, 2021 10:46 Inactive
Copy link
Member

@wirew0rm wirew0rm left a comment

Choose a reason for hiding this comment

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

Looks good from my POV. Improves the default behavior a lot while allowing different levels of customization. 👍

@RalphSteinhagen RalphSteinhagen merged commit 215c82f into master Jul 6, 2021
@RalphSteinhagen RalphSteinhagen deleted the datasetFormatter branch July 6, 2021 16:16
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.

Number formatting for automatic dataset names
3 participants