Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-166987980
@nitin-maharana, thanks for the update, I will just call your attention to
a few points:
First of all, why did you open a new PR? I know you closed the other one,
but IMHO it makes my job harder. The other PR had all of our conversions; you
should have kept working there. It would be easier to track everything that was
discussed if everything was at the same PR. Now please, keep with this one, no
need to come back to the one you already closed or to open a new one after my
suggestions.
I noticed that you extract the code to a method as I suggested and created
some test cases. You also add the ârequired = trueâ as Daan suggested; that
is good.
Now about the method âgetVolumeNameFromCommandâ you created. Please
note that // is not the proper way to create a Java doc, please replace the //
by the proper mark of Java docs
/**
* this is a java doc
**/
/*
*This is a block comment
*/
// this is a line comment
About the Java doc I would also encourage you to detail a little bit more
the method such as:
/**
* This method retrieves the volume name from the CreateVolumeCmd object.
* If the method CreateVolumeCmd#getVolumeName() returns null, empty or
blank, It will be generated a random name using getRandomVolumeName().
**/
I do not know the proper notation of java doc to make reference to java
classes and methods, so you will need to google it.
Now about the tests, please remove those comments over method names. I
believe they are not needed as the test method name is self-explained.
About the âdocs.jsâ, please improve the sentence you wrote. 'Enter a
unique volume name; if a name is not provided, a random name will be created'
Now, I draw your attention again to the idea of sticking to the same PR
were we started reviewing and chatting. If another reviewer comes to help with
this new PR, she/he would not know about the blank case I told you in the other
PR. The reviewer might even give a LGTM without even noticing that this PR is
incomplete.
Having said that, could you add a test case to check the case in which
users enter blank strings in the volume name? And of course, fix that in the
code; otherwise, the test methods would not work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
