Correcting problems reported on Issue #212#214
Correcting problems reported on Issue #212#214vgartner wants to merge 1 commit intomysticfall:masterfrom
Conversation
Addresses and corrects problems reported on Issue mysticfall#212
|
I've made a few comments about your suggestions, so I'd like you to review them and let me know how you think. Thanks much for the contribution! 😃 |
|
Hi, you're welcome. |
|
@vgartner I might be doing something stupid myself, since it's the first time I've used the new review feature 😅 Anyway, aren't my comments visible above? On my end, I can see them along with the relevant lines from your code. If you can't, maybe you can try switching to Files Changed tab and see if you can read them there. |
|
Hi @mysticfall
|
| for (Exp set : sets) { | ||
| try { | ||
| Hierarchy hierarchy = quaxUtil.hierForExp(set); | ||
| hiers.add(hierarchy.getName()); |
There was a problem hiding this comment.
I agree that it's safer to use unique names than simple names for look up.
But I guess you might one to check other places that still use simple names when they deal with hiers and hierarchyMap variables, like those at 500-501 line, for instance.
| oncomplete="PF('hierarchyConfig').show();" | ||
| title="#{msg['button.hierarchyConfig.tooltip']}"> | ||
| <f:param name="hierarchy" value="#{node.name}" /> | ||
| <f:param name="hierarchy" value="#{node.caption}" /> |
There was a problem hiding this comment.
I don't think we should rely on caption when we look up hierarchies, since there could be duplicates.
I'm not too sure about the exact problem with using name, but if there's such a problem, can't we use uniqueName instead?
| if (hierarchy == null && hierarchyName != null && model.isInitialized()) { | ||
| this.hierarchy = model.getCube().getHierarchies() | ||
| .get(hierarchyName); | ||
| for (Hierarchy h : model.getCube().getHierarchies()) { |
There was a problem hiding this comment.
Please read my other comment about using uniqueName instead of caption. And if there's a valid reason why we shouldn't use uniqueName there, then I suppose we shouldn't use it in Quax either.
Please see following lines at Quax.java:2926:
hierarchy = getModel().getCube().getHierarchies().get(name);With your changes, the name parameter became uniqueName instead of simple name. So, it's basically the same code as the one you've changed here.
There was a problem hiding this comment.
And please make it sure that the modified code follows the same formatting rules as the rest of the codebase. The project uses tabs for indentation, but the modified part seems to have 8 spaces per indent.
| } | ||
| } | ||
| } | ||
| // this.hierarchy = model.getCube().getHierarchies() |
There was a problem hiding this comment.
You don't have to leave them with comments, since we can always restore from history if needed :)
|
@vgartner Oh, my.... I feel stupid! 😓 I just realized that I need to actually 'submit' the review after I write individual comments. I believe now you should be able to see it. Sorry for the confusion! |
2 participants
Addresses and corrects problems reported on Issue #212