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

Umple command line compiler requires the generator language specified with exact case #602

Closed
Nava2 opened this Issue Aug 26, 2015 · 10 comments

Comments

Projects
None yet
5 participants
@Nava2
Member

Nava2 commented Aug 26, 2015

Originally reported on Google Code with ID 704
Owned by @umple


What steps will reproduce the problem?

  1. Run the command line compiler on a php file with java -jar -g php ~/yourphpfile.php
  2. Check result

What is the expected output? What do you see instead?
The command line compiler expect 'Php' and won't generate if 'php' is specified instead

Languages shouldn't be case sensitive (i.e. 'php' should be just as valid as 'Php')

Please use labels and text to provide additional information.


Reported by jzwiep on 2015-04-13 04:32:43

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Jan 3, 2016

Very easy and quick to do: Convert all case variants to the expected one in main program.

See if there is any automated testing; ideally add automated testing for such options.

Make sure you understand the two main programs firs, so as to apply this to the correct one.

@bitwizeshift

This comment has been minimized.

Contributor

bitwizeshift commented Jan 13, 2016

The generator string seems to only ever be used in the GenerateTarget object. Would it perhaps be best to convert the string in GenerateTarget with a before constructor statement instead of in the main?

If I'm understanding the code correctly, doing it this way could correct invalid inputs in both the PlaygroundMain and UmpleConsoleMain (and any other components that make use of GenerateTarget) without having to re-detect the parameters supplied to the -g/--generate arguments (which is handled with optParse(...)).

I can make the change in the main if that's still the preferable option; I was just reading into how parameters are handled and figured I'd ask.

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Jan 13, 2016

Yes, converting the string in GenerateTarget might well be the best solution to explore. The only catch I see is that there will be a need to list all the possible targets as they use inconsistent capitalization. Can you think of a way to avoid this? In the general case, the name of the target is comes from the name of a class.

@bitwizeshift

This comment has been minimized.

Contributor

bitwizeshift commented Jan 13, 2016

I'm still searching the Umple codebase for this, but I'm hoping to find an instantiated list of all supported target-languages (with proper capitalization) -- preferably something automatically generated. Something like that would be useful to compare against, and would be scalable if ever new languages were added.

If something like that exists, then the check can simply be done by iterating and comparing against all known options using equalsIgnoreCase, and correcting/exiting the loop if a match is discovered.

Another possibility may be to somehow test the string against the capitalization of an object with the name cruise.umple.compiler.{0}Generator, where {0} will be the prefix string (since this is currently how the generator is created in newGenerator(...)). The only thing is it's not a particularly nice solution, as anything with that form can be tested (including non-generators, if they are to exist)

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Jan 13, 2016

On the command line there is a result displayed by the --help option. One approach would be to create a master list that is used both by that option and also by your validator. Currently what Umple does is search for a class with the name of the generator proposed. But then you just get a failure if it is wrong.

There is also a list of targets in the grammar used for the 'generate' command. That could be used as the basis for both the command line help and your improvement

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Jan 13, 2016

Since, we have two diiferent dimensions here (logical and user interface), It would be nice to keep both separated and worrking properly. I would suggest to make sure all decisions made in logical part are based on lowercases (except cases we really need upper cases as). Afterwards, in the user interface side (command line) we check the parameters if they come correctly but in uppercases (or lowercases) we don't run them, but we detect them and ask users if they mean this parameter or not. This is the best practice in order to make sure users are not going to run their scripts again and also give a valid parameter to the logical part of Umple. By doing this, we achieve a solid logic which will work propely for all other cases and have the best flexibility for users.

@bitwizeshift

This comment has been minimized.

Contributor

bitwizeshift commented Jan 13, 2016

@TimLethbridge
I figured there'd be a list of generators somewhere, I just didn't think to check umple_core.grammar. Thanks!

@vahdat-ab

...we don't run them, but we detect them and ask users if they mean this parameter or not

Are you suggesting to not automatically correct strings of different cases, but to only recommend the correct one to the user?

I would suggest to make sure all decisions made in logical part are based on lowercases (except cases we really need upper cases as)

Wouldn't all the logic side have to be in the proper case (upper/title-case), since it's used to look up the generator by the class's fully qualified name?

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Jan 13, 2016

Yes, I think automatically changing the parameters is not a good approach because it's not a scalable approach and can make issues when Umple is growing more.
Yes, we look at the named of generators and consider them as parameters.
BTW, even though the comment in the issue says the language should not be case sensetive, but this is a different case. These are parameters for command lines and we can have case sensetive parameters. They key part is having a smart interpreter that detects this problem and inform users.
Regarding logical part, we can keep them. In that case the users are supposed to get a notification when they try to run a command like this

java -jar -g php ~/yourphpfile.php

The notification can be sth like this "do you mean Php" or sth close to this which gives more information.

@bitwizeshift

This comment has been minimized.

Contributor

bitwizeshift commented Jan 13, 2016

Alright sounds good, thanks for clearing that up!

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Jan 13, 2016

Hmm. Actually I am not sure I agree with Vahdat on this. I was in favour of the autocorrect, since it is hard to remember Php or Cpp vs. the lowercase version. My thought was that we would grab the list in the grammar rule and compare the lowercase version of the parameter with the lowercased version of each element in the grammar rule. When there is a match, then take the case as specified in the grammar rule as we know this is correct.

This would help the user more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment