Skip to content
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

Use enumeration for field type #1531

Merged
merged 12 commits into from
Jun 21, 2015
Merged

Use enumeration for field type #1531

merged 12 commits into from
Jun 21, 2015

Conversation

leleuj
Copy link

@leleuj leleuj commented May 26, 2015

Here are the changes to be able to choose the 'enum' type for the model and in the view.

@jdubois
Copy link
Member

jdubois commented May 27, 2015

Thanks @leleuj

I've just tested and I have a compilation error on the Enum class, and anyway I can't find where your generate it -> should this class already exist? If so, why do you ask for its values?

I think you should generate that class.

@leleuj
Copy link
Author

leleuj commented May 27, 2015

I had in mind that the enum was manually generated before being used by the generator, but it sounds like a mistake now.

I will generate it along the other elements...

@leleuj
Copy link
Author

leleuj commented Jun 1, 2015

@jdubois : just added the automatic enumeration generation.

enumInfo.enumName = fieldType.substring(lastDot + 1);
enumInfo.enumValues = field.fieldValues;
this.template('src/main/java/package/_Enum.java',
'src/main/java/' + fieldType.replace(/\./g, '/') + '.java', enumInfo, {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look good, you should have the app's package here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I request the user to enter a class name, but what I mean, is to enter a "full class name with package" (line 198). I will change the text to make things clearer if you agree?

@jdubois
Copy link
Member

jdubois commented Jun 8, 2015

I tried to merge this PR but I had too many issues:

The package name: I would like to have:
this.template('src/main/java/package/domain/enumeration/_Enum.java', 'src/main/java/' + this.packageFolder + '/domain/enumeration/' + enumInfo.enumName + '.java', enumInfo, {});

Then this needs to be imported into the related entity, or it won't be able to compile.

I also don't think the tests are good -> I don't have a solution for this, but here the user needs to have at least 2 items in his enumeration, and I'm sure this will fail as some people will only put 1 at the beginning

I don't think this will currently work with DTOs -> this is normal, as I added DTOs after your PR, but still that makes a new issue

In this code, it's supposed to work with MongoDB -> did you test it? I would be surprised that it works, but didn't try it

@leleuj
Copy link
Author

leleuj commented Jun 9, 2015

I guess people should be able to enter the full name of the enumeration as it can be shared between model entities.

I will re-test with only one value in the enumeration but it should work: https://github.com/jhipster/generator-jhipster/pull/1531/files#diff-a1ae891973b6d5fc4724f25a590d36d2R77

I will check if it works with Mongo and DTO as well.

@leleuj
Copy link
Author

leleuj commented Jun 9, 2015

I fixed the text with "What is the full class name (with package) of your enumeration?" to allow people to choose where to save the enumeration.

I upgraded to the latest version of jhipster and tested it again successfully. Unless I'm mistaken, DTO are properly created as the required validation rules is applied if necessary. I created a new application sample and it works with Mongo as well.

Is it ok for you now?

@leleuj
Copy link
Author

leleuj commented Jun 15, 2015

@jdubois : are you ok with the pull request?

@leleuj
Copy link
Author

leleuj commented Jun 17, 2015

@jdubois I'm going to be in holidays for 2 weeks: does this PR finally meet your expectations?

@jdubois
Copy link
Member

jdubois commented Jun 17, 2015

Hi @leleuj I'm sorry I didn't have the time to review it. I'll try to do it soon.

@leleuj
Copy link
Author

leleuj commented Jun 17, 2015

Thanks. At worst, it'll have to wait for my return...

@jdubois
Copy link
Member

jdubois commented Jun 17, 2015

For the package name: we never ask for it, there is always a "default" package name. That's the case for entities, services, etc.
This is also useful when use several sub-generators, as you know where the other files are.

Anyway, I think it's better to have the same mechanism here, so I'd like to have a fixed package name for enums.
Problem is, which one should it be? My guess would be an "enumeration" subpackage under the "domain" package. Would that be good for you?

@leleuj
Copy link
Author

leleuj commented Jun 17, 2015

OK. Let's be consistent. I will update things to save the enumeration in the "domain.enumeration" package.

@leleuj
Copy link
Author

leleuj commented Jun 17, 2015

I just updated the PR with your feedback and retested it with two apps: one for Mongo, one for SQL DB, using one value and multi values enumerations.

@jdubois jdubois merged commit 510b52b into jhipster:master Jun 21, 2015
@jdubois jdubois added this to the Untracked PRs milestone Oct 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants