"FORGE-1780: New UI Command" should default commandName based on type name camelcase #451
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -72,6 +74,12 @@ public Result execute(UIExecutionContext context) throws Exception | |||
| { | |||
| JavaResource javaResource = context.getUIContext().getSelection(); | |||
| JavaClassSource command = javaResource.getJavaType(); | |||
|
|
|||
| if (Strings.isNullOrEmpty(commandName.getValue())) | |||
There was a problem hiding this comment.
Can you add a test with this change?
|
@gastaldi NewUICommandWizard doesn't have specific test? Can where to do my test? Do I need create a new test class? |
|
Build 395 outcome was FAILURE using a merge of c719015 Build problems:Failed tests detected Failed tests |
|
@danielsoro, yes you need to create a new test class for the command. I believe we forgot to add one |
danielsoro
changed the title
|
Build 398 outcome was FAILURE using a merge of d81c416 Build problems:Process exited with code 1 |
| * | ||
| */ | ||
| @RunWith(Arquillian.class) | ||
| public class NewUiCommandWizardTest |
There was a problem hiding this comment.
Class name should be NewUICommandWizardTest, to match NewUICommandWizard class (the "i" should not be lowercased)
|
Build 400 outcome was FAILURE using a merge of cbc872e Build problems:Failed tests detected Failed tests |
|
Build 399 outcome was FAILURE using a merge of cbc872e Build problems:Execution timeout |
|
Build 438 outcome was FAILURE using a merge of cbc872e Build problems:Failed tests detected Failed tests |
|
Looks good 👍 |
|
Ah, there is a catch: Most of the commands/wizards created have a "Command" suffix (eg: BatchNewJobCommand) and it would be nice to remove this suffix from the command name. Having said that, the parseCamelCaseToSeparatedByHyphens method can be renamed to something like calculateCommandName |
|
"Wizard" and "WizardCommand" are common suffixes too, so maybe these should be considered as well |
|
Hmm, Ok.. I'll change later. Att;
|
|
Thx George. :) Att;
|
| char charValue = value.charAt(index); | ||
| if (Character.isUpperCase(charValue)) | ||
| { | ||
| builder.append("-").append(Character.toLowerCase(charValue)); |
There was a problem hiding this comment.
it should append "-" only if a lowercase was previously found Eg: NewUICommand seems to be calculated as "new-u-i" but it should be "new-ui-command"
There was a problem hiding this comment.
Hmm.. sure. I'll review the method. Today I have more time to do this.
There was a problem hiding this comment.
@gastaldi Ok, but your example.. How do I apply ? Because between UI and Command doesn't exist a lowercase previously. Command was removed, not exist Command into commandName when I not specified the commandName parameter or when ends with Command. In you example it's easy, my method return: new-ui
But when I have for example, this parameter: NewUITesteCommand.. What my method should return? new-ui-teste, new-uitest or new-u-i-test. Append "-" only if a lowercase was previously found not a better form, this example doesn't pass. [I hope that you understand this text]
There was a problem hiding this comment.
Ah, I see. In this case, the default should be "new-uiteste"
Em 06/05/2014, às 19:07, Daniel Cunha notifications@github.com escreveu:
In addons/impl/src/main/java/org/jboss/forge/addon/addons/ui/NewUICommandWizard.java:
if (Strings.isNullOrEmpty(value)){throw new IllegalArgumentException("It isn't possible to parse a null value");}
if (Character.isUpperCase(value.charAt(0))){value = value.replace(value.charAt(0), Character.toLowerCase(value.charAt(0)));}
for (int index = 0; index < value.length(); index++){char charValue = value.charAt(index);if (Character.isUpperCase(charValue)){builder.append("-").append(Character.toLowerCase(charValue));But when I have for example, this parameter: NewUITesteCommand.. What my method should return? new-ui-teste, new-uitest or new-u-i-test. Append "-" only if a lowercase was previously found not a better form, this example doesn't pass. [I hope that you understand this text]
—
Reply to this email directly or view it on GitHub.
|
Build 409 outcome was FAILURE using a merge of 1c53679 Build problems:Execution timeout |
|
Build 447 outcome was FAILURE using a merge of 1c53679 Build problems:Failed tests detected Failed tests |
|
Build 410 outcome was FAILURE using a merge of 1c53679 Build problems:Failed tests detected Failed tests |
|
Build 450 outcome was FAILURE using a merge of 1c53679 Build problems:Failed tests detected Failed tests |
|
Build 413 outcome was FAILURE using a merge of 1c53679 Build problems:Process exited with code 1 |
…e name camelcase. Create test class: NewUiCommandWizardTest FORGE-1780: Created method to create default commandName based on type name camelcase. Create test class: NewUICommandWizardTest FORGE-1780: Created method to create default commandName based on type name camelcase. Create test class: NewUICommandWizardTest FORGE-1780: Created method to create default commandName based on type name camelcase. Create test class: NewUICommandWizardTest
|
Build 451 outcome was FAILURE using a merge of 4544d7e Build problems:Failed tests detected Failed tests |
|
Build 452 outcome was FAILURE using a merge of 4544d7e Build problems:Failed tests detected Failed tests |
|
Build 414 outcome was FAILURE using a merge of 4544d7e Build problems:Process exited with code 1 |
|
Build 415 outcome was FAILURE using a merge of 4544d7e Build problems:Failed tests detected Failed tests |
|
Build 454 outcome was FAILURE using a merge of 4544d7e Build problems:Failed tests detected Failed tests |
| @@ -156,4 +162,35 @@ protected String getType() | |||
| { | |||
| return "UI Command"; | |||
| } | |||
|
|
|||
| static String calculateCommandName(String value) | |||
There was a problem hiding this comment.
This works fine in Shell, however it doesn't look that good in Eclipse. I think a better choice would be to use the XXX: yyy pattern (eg: 'Batch: New Job' for the BatchNewJobCommand class), as Forge will automatically add dashes when required
There was a problem hiding this comment.
Sorry @gastaldi, but I didn't understand very well your observation.
There was a problem hiding this comment.
Take for example: BatchNewJobCommand: If you see the command in Eclipse (Ctrl+4) you'll see an option named "batch-new-job". Forge "shellifies" command names by replacing spaces and special chars to "-" and lowercasing them on shell. So, if in this example, if this method returned "Batch: New Job", that is how it would appear in the Eclipse dialog (Ctrl+4) and it would appear "batch-new-job" in shell. Much clearer?
There was a problem hiding this comment.
In the same pattern, NewUITestCommand would be translated to "New: UITest", which makes you think of rephrasing your class name :)
|
Build 417 outcome was FAILURE using a merge of 4544d7e Build problems:Process exited with code 1 |
gastaldi
added a commit
that referenced
this pull request
May 8, 2014
"FORGE-1780: New UI Command" should default commandName based on type name camelcase
|
We can figure that naming strategy later. This looks good for now |
|
@gastaldi Ok! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FORGE-1780: UIStrings to create default commandName based on type name camelcase. Change in NewUICommandWizard to use UIStrings, case commandName null.