add option to change GENERATION_SUFFIX #1280

dodgex · 2014-12-22T10:16:13Z

this allows to set the generationSuffix via options. at the moment the same valule is used for all generated stuff (classes, fields, methods) where it before was _. But for a first look i already opened the PR.

dodgex · 2014-12-22T10:54:16Z

damn @yDelouis you got me. i only searched for "_". that explains why there where so less results.

stupid me... i'll update that.

yDelouis · 2014-12-22T10:57:41Z

I removed my notes because we don't have to change the _ on private or static methods. We could even remove them (on init_() or getInstance_() for exemple).
We only have to change this for classes and things which may be conflicting.

Anyway, do you plan to add some tests to be sure that everything work together ?

dodgex · 2014-12-22T12:11:38Z

the comment at https://github.com/excilys/androidannotations/pull/1280/files#diff-29b59400d9e85af5aa52fae67c90103eR637 is still valid i think. this converts MyPref_ to MyPref but that won't work if it is eg. MyPref_Impl due to a configured suffix.

should i replace the other _ that are still there with another suffix constant or should we keep them untouched?

tests... yeah. there should be some but i have no idea yet how.

yDelouis · 2014-12-22T12:33:41Z

I think we could keep the '_' for methods and fields. A method named initImpl would be very weird.

dodgex · 2014-12-22T12:46:46Z

updated the PR to to use the new suffix for IntentBuilder and FragementBuilder as well as fixing the code from the above comment.

yDelouis · 2014-12-22T13:44:14Z

...ns/androidannotations/src/main/java/org/androidannotations/helper/AndroidManifestFinder.java

I think there is an issue if the generation suffix has more than 1 character.

WonderCsabo · 2014-12-22T13:53:32Z

I think we should use two constants, one which can be changed and another
which is used internally (as Yoann is suggesting).

yDelouis · 2014-12-22T13:53:38Z

...droidannotations/src/main/java/org/androidannotations/holder/ReceiverRegistrationHolder.java

yDelouis · 2014-12-22T14:00:29Z

@WonderCsabo is right. We should have a subclass suffix which can be changed by the processor options. And another suffix for generated fields and method to prevent conflicts with the super class ones. This could be _ as it is now.

Then, for inner classes of generated classes (IntentBuilder, FragmentBuilder) and for static methods (getInstance) we could remove the suffixes.

WonderCsabo · 2014-12-22T14:33:00Z

@yDelouis i do not think we should remove the suffices from anywhere. The suffix also clearly shows that the class/etc. is generated by us, which is very important if you have a bigger project and/or multiple people are working on it.

WonderCsabo · 2014-12-22T14:51:59Z

...notations/androidannotations/src/main/java/org/androidannotations/helper/ModelConstants.java

We have to validate the suffix here, because the user can pass a string which is not valid in Java identifiers.

@dodgex you can use SourceVersion.isName method to the the validation.

and what SourceVersion value to use? SourceVersion.RELEASE_6 or SourceVersion.RELEASE_7?

https://github.com/excilys/androidannotations/blob/326fe9666df37284245c4e2706f4c042f0b396c1/AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/AndroidAnnotationProcessor.java#L285

BTW, isName() is static, isn't it? The enum value does not matter.

So if (!SourceVersion.isName(classSuffix) || classSuffix.contains("."))

ah. missed that static... well i'll update the PR when i get the time.

yDelouis · 2014-12-22T14:55:26Z

About static method, you have to write MyClass_.getInstance() so you can see that it comes from generated class.
About inner classes, you generally import the outer class and write MyClass_.InnerClass in the code. So, again, it is clear that it has been generated.
More over, we already generate static methods which doesn't have the suffix, like the intent method of an EActivity. And we should be consistent.

WonderCsabo · 2014-12-22T14:56:40Z

Seems reasonable.

But, this will break existing client code. :( We should only do the breaking changes if it is really necessary.

WonderCsabo · 2014-12-22T15:11:10Z

About testing: I think the functional test project almost tests this correctly. Maybe we should add some compile time tests to check reading the options and scanning the manifest etc works well.

dodgex · 2014-12-23T10:26:15Z

so i'm going to update the PR:

classes generated will get the configurable suffix
fields/methods a new internal not configurable(?) suffix
static imports of generationSuffix()
i will not change the default behaviour of the generated code so that there should be no breaking change (not removing the suffix from getInstance_() or IntentBuilder_).

WonderCsabo · 2014-12-23T10:28:57Z

i will not change the default behaviour of the generated code so that there should be no breaking change (not removing the suffix from getInstance_() or IntentBuilder_).

Let's wait with that one, because we did not discuss that problem, i only raised it.

The remainder are OK.

And plus one thing: you have to validate the configurable suffix and throw an exception if it cannot be a valid Java class suffix.

dodgex · 2014-12-23T19:58:23Z

updated the PR. all _ (where needed) should now be replaced with classSuffix() or generationSuffix().

WonderCsabo · 2014-12-23T23:46:55Z

...notations/androidannotations/src/main/java/org/androidannotations/helper/ModelConstants.java

We should also have to be sure the string is not empty (zero-length), because it will result in duplicate class names.

added a trim() and also checked the length.

WonderCsabo · 2014-12-27T23:16:26Z

Please rebase this.

dodgex · 2014-12-28T16:55:46Z

rebased.

WonderCsabo · 2014-12-28T17:08:53Z

You have Checkstyle violations. Moreover, i think the rebase was not really successful, as i removed the duplicate imports earlier, and it seems you re-added them?

dodgex · 2014-12-28T17:11:54Z

yeah. on my way to fix that.

dodgex · 2015-04-26T13:32:55Z

working on a test. currenlty we have classSuffix used for class names and generationSuffix for fields and methods.

in the current implementation only classSuffix is changable with a parameter. should i add an option for generationSuffix and test it or do we only provide the ability to change classSuffix as the user should never get in touch with anything that uses generationSuffix as it is only used for internal methods & fields?

WonderCsabo · 2015-04-26T13:37:11Z

generationSuffix should be never changed, the purpose of it is to make the suffix consistent through the codebase. It is enough to test classSuffix change. Refactoring generationSuffix is already tested in the functional test project and it passed. :)

dodgex · 2015-04-26T13:43:32Z

test added

WonderCsabo · 2015-04-27T15:55:16Z

@dodgex i am wondering, should'nt we remove the _ or generationSuffix() suffices from the locals and params?

WonderCsabo · 2015-04-27T15:58:35Z

...otations/androidannotations/src/main/java/org/androidannotations/holder/EFragmentHolder.java

I do not think it should use classSuffx, but generationSuffix instead.

why? its an generated class that is accessible for the user. so FragmentBuilderImpl is better than FragmentBuilder_ actually i'd rename the FragmentBuilder api class (to e.g. AbstractFragmentBuilder) and call the generated class FragmentBuilder so this would be CLEAN.

same to other Builder classes.

I do not agree. Generally we use this impl classes when there is an interface for a specific work, and then we provide an implementation for that work. Here, we do not provide implementation for the work, but we generate always the interface actually, too.

ok. then i'll change that.

WonderCsabo · 2015-04-30T14:53:45Z

@dodgex any update on this?

dodgex · 2015-05-01T13:47:19Z

@WonderCsabo i'm currently not at home. Will try to update this PR next week.

…_suffix Add option to change generation suffix

WonderCsabo · 2015-05-03T18:51:14Z

Thanks for your work on this! Can you update the wiki?

WonderCsabo · 2015-05-03T21:11:30Z

Thanks to @dodgex for editing the wiki.

chpasha · 2015-06-17T14:23:10Z

i do not think we should remove the suffices from anywhere. The suffix also clearly shows that the class/etc. is generated by us, which is very important if you have a bigger project and/or multiple people are working on it

Hi guys, regarding "clearly showing that the class is generated by us" - why not use @generated annotation which was meant exactly for that? http://docs.oracle.com/javaee/5/api/javax/annotation/Generated.html . As far as I understand, it is enough to modify setGeneratedClass-Method inside BaseGeneratedClassHolder adding a line like this

generatedClass.annotate(Generated.class)
.param("value", AndroidAnnotationProcessor.class.getName())
.param("date", getISO8601StringForDate(new Date()));

maybe even use a specific holder FQN instead of generic AndroidAnnotationProcessor.class

WonderCsabo · 2015-06-17T14:50:51Z

@chpasha that annotation is not available in the Android runtime, and will cause problems if we add it, despite its source only retention. BTW lombok has a lombok.addGeneratedAnnotation property for this reason.

chpasha · 2015-06-17T15:03:44Z

I didn't know that, so I just was hacking around and got no errors when running some test android project with this annotation applied. Which problems will it cause since it is discarded by compiler?

WonderCsabo · 2015-06-17T15:12:31Z

Okay, it seems @Generated is not a problem, i mistaken it for @ConstructorProperties.

In that case, i agree with you @chpasha.

chpasha · 2015-06-17T15:18:38Z

great :)

WonderCsabo · 2015-06-30T10:07:19Z

@chpasha unfortunately i remembered correctly. Transfuse and Parceler for instance adds an own version of @Generated, because that annotation is not available in the android.jar.

chpasha · 2015-06-30T11:47:14Z

I'm not sure I get it :) . This annotation is not included in apk because of it's retention policy. So why should we care about if it is present in android.jar? Like I said, for the sake of experiment I added this annotation to the generation process and it doesn't cause any issues at all.

WonderCsabo · 2015-06-30T11:50:26Z

If the annotation is not in the jar, the compilation will fail. This is the case for Eclipse users for instance.

chpasha · 2015-06-30T12:03:41Z

I'm not sure I understand what it has to do with eclipse :) . I must be missing something, but the annotation is present in JDK6 and later, so it is available during compilation. But don't get me wrong, I'm just curious, I don't insist on introducing or implementing anything as long as I can maintain my own fork :)

P. S. actually it was added in dagger2 square/dagger#265 as well ;)

WonderCsabo · 2015-06-30T12:06:20Z

@chpasha it is indeed JDK6, but that is not in the classpath in case when building an Android project in Eclipse. Only the android.jar is on the classpath.

chpasha · 2015-06-30T12:09:31Z

Ok, then maybe adding an annotation processing option that disables @generated makes sense? Just for the sake of completeness, since this annotation has not much real value anyway.

WonderCsabo · 2015-06-30T12:11:18Z

Yeah, that makes sense, and as i said Lombok does the same. However we have to disable it default, to not break silently compilation for Eclipse users. Which means AS users will also not find the option, and never generate these annotations. :(

yDelouis reviewed Dec 22, 2014
View reviewed changes

...droidannotations/src/main/java/org/androidannotations/holder/ReceiverRegistrationHolder.java Outdated

Copy link

Contributor

yDelouis Dec 22, 2014

Choose a reason for hiding this comment

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

Same here.

WonderCsabo reviewed Dec 22, 2014
View reviewed changes

WonderCsabo reviewed Dec 23, 2014
View reviewed changes

WonderCsabo mentioned this pull request Dec 25, 2014

ValidatorParameterHelper fluent API #1284

Closed

WonderCsabo reviewed Apr 27, 2015
View reviewed changes

dodgex added 2 commits May 3, 2015 20:04

add option to change GENERATION_SUFFIX

edc9af1

add test case for classSuffix parameter

5aacf3e

WonderCsabo added a commit that referenced this pull request May 3, 2015

Merge pull request #1280 from dodgex/1209_option_to_change_genreation…

87f4233

…_suffix Add option to change generation suffix

WonderCsabo merged commit 87f4233 into androidannotations:develop May 3, 2015

WonderCsabo added this to the 4.0 milestone May 3, 2015

WonderCsabo added WikiUpdateNeeded and removed WikiUpdateNeeded labels May 3, 2015

dodgex deleted the 1209_option_to_change_genreation_suffix branch May 24, 2015 13:45

add option to change GENERATION_SUFFIX #1280

add option to change GENERATION_SUFFIX #1280

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh E3AB oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone