expose more config for bubble plot #781
Conversation
There was a problem hiding this comment.
It looks like you use tabs where the rest of the code uses spaces and some other formatting that's not consistent. If you run mvn compile one time it will install a git pre-commit hook so that whenever you commit code the code will be automatically formatted and you don't have to worry about formatting
@benmccann I manually format the change in Eclipse using google code format plugin at https://github.com/google/google-java-format |
benmccann
changed the title
benmccann
changed the title
|
Hey @rayeaster I'm not sure the formatting has been updated yet. It still looks like it uses tabs instead of spaces |
now it should looks good with the google formatter. Thanks for taking time on review |
|
Thanks for that! I'm concerned that the plugin didn't work automatically for you on Windows. It should work there. I saw that they include a few fixes for Windows in some of the recent releases, so I've upgraded it to the latest release: 9d5fce1. Would you be able to test it again and let me know if it works for you now or what error message you get if it doesn't? I don't want other users to run into this problem. Thanks! |
yes, |
| String title, Table table, String xCol, String yCol, String sizeColumn) { | ||
| String title, | ||
| Table table, | ||
| String xCol, |
There was a problem hiding this comment.
It seems potentially confusing that there's a parameter xCol and xColumn and only one is used. It's not clear which takes precedence. Can you remove one of the two parameters? (same with yCol / yColumn). That would also probably fix the Codacy warning that's being generated
There was a problem hiding this comment.
I agree with the above. I've kept postponing commenting on this PR because it seems to me adding more and more create methods is not a good solution in a way; the other is that this method seems something that could be perhaps private and expose to the users something more clean so they don't have to think about the difference between xCol and xColumn. Might be simple of getting rid of the table argument altogether and just send columns.
There was a problem hiding this comment.
@benmccann @emilianbold I updated PR w.r.t above comments. Thanks for review.
|
This looks good to me, unless this PR gets some other feedback in the following few days I will merge it. |
|
Seems fine to me |
|
With 2 +1s no need to wait. |
expose more configurable options for bubble plot.