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

stupid me... i'll update that.

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 ?

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.

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

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

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

@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.

@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.

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.

Seems reasonable.

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

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.

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_).

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.

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

Please rebase this.

rebased.

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?

yeah. on my way to fix that.

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?

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. :)

test added

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

@dodgex any update on this?

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

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

Thanks to @dodgex for editing the wiki.

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

@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.

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?

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

In that case, i agree with you @chpasha.

great :)

@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.

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.

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

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 ;)

@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.

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.

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. :(