-
Notifications
You must be signed in to change notification settings - Fork 87
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
Keep arguments order as declared #25
Comments
The parameters should always be in alphabetic order (field names), isn't it? |
Hm.. You are right. At least now I know the rules how args are ordered) But what is the point of forcing order to alphabetic? |
In annotation processing there is no guarantee that you get the list of fields in the same order as you have declared them in your source file. As far as I know that is a compiler implementation detail. i.e. Eclipse JDT order them alphabetic. There are some workarounds and hacks to get the original declaration order like https://github.com/google/auto/blob/master/value/src/main/java/com/google/auto/value/processor/EclipseHack.java By ordering them, I can guarantee that I always have a predictable result and to me the order of parameters is not that important as long as parameters are named correctly. |
Interesting point, didn't know that. Of course this can be fixed with explicit parameter like 'order' or 'index' that user can add to |
that would be a possible optional feature, but does order really matters that much? How often do you change or add arguments to a fragment? From my point of view there are not many people having trouble with the order. |
Not so often. If you think that such feature would be overengineering you can close this Issue. |
I will keep it open, if more people have interest in such a feature then I will consider to implement it. |
Since there is no guarantee of ordering by default, I would consider extra annotation params superfluous, and am ok with current alpha order. By having lots of child fragments inheriting args and providing their own required/not arguments, fragment construction becomes a mix and match when viewed from an initializing class. Working on a team/with consistent apis @Yarikx, each person working on a child fragment benefits in readability from preserving order based on initialization, allowing you to build on params for complex fragments. // ordered example: Frag06 extends Frag05 extends Frag04...
new Frag06Builder(constructionId, isLayoutTablet, isBackStackable,
Frag04ReqArg, Frag05ReqArg, Frag06ReqArg).build();
// otherwise if using alpha ordering, inherited Frags mess the constructor cumulatively to:
new Frag06Builder(constructionId, Frag04ReqArg, Frag05ReqArg,
Frag06ReqArg, isBackStackable, isLayoutTablet).build(); |
I understand your problem, but does your suggestion solves your problem? new Frag06Builder(constructionId, Frag04ReqArg, Frag05ReqArg,
Frag06ReqArg, isBackStackable, isLayoutTablet).build(); From my point of view the problem is still the same: You have to set the required parameters according the alphabetical order and then the optionals in alphabetical order. You just have divided it into two pieces that you have to order (first the requires, then the optional arguments). Your example is misleading since you have used variable names that makes your sample look as a good solution. Update: I had an error in reasoning, optional arguments are not part of the constructor The problem with changing the order of arguments is that it will break backward compatibility. If you would use the newest version of FragmentArgs with changed argument order in an already existing app that have used a previous version of FragmentArgs you have to check your whole app for changes of arguments order. It's even harder to detect changes if your arguments are of the same type i.e. Hence I would consider changing arguments order only for a very very good reason. Hence I still believe that an alphabetical order is the best way. We could start to discuss about other solutions like adding a integer property to the annotation to manually influence the order like this: However, from my point of view this problem is something specific to you and your company. |
I did not quite understand the first part of your post, apologies, my suggestion was the first ordered example, the second was an illustration of what could happen with each inherited iteration offering a mix of required variables that start with a, b, c... Besides that, I agree with your point on backwards compat, and @ Arg(order) being a poor approach for inherited arguments for the same reason AnnotatedAdapter explains "...is that the I hope that clarified my perspective. |
Just an idea, wouldn't a Fluent Interface for non-optional arguments as well work here ? Ordering is no longer an issue although the code slightly increases. But I would prefer the extra code over the chore of reordering whenever I rename or change a parameter. Adding an extra check at runtime to make sure all the arguments are set would solve all the issues. |
@Gautham jep, I also thought about such a solution ... At least from my point of view a big issue is that you loose "compiletime" evaluation because you do the check if all required arguments are there at runtime. Imho this is even more problematic since you change the argument, i.e. in a base fragment class and all your app build but you have to verify that all fragment extending from that base fragment class still work at runtime. |
Actually I think it's possible to get the order in which the fields are declared. Instead of getting the elements annotated with I suppose there might be some overhead using this approach if the file is very big and has many elements. |
That's true nowadays. FragmentArgs was build a few years ago when the majority of android development took place in Eclipse IDE. Eclipse used its own java compiler, where this wasn't true. However, there are also some other things to consider like inheritance i.e. a In my opinion alphabetic order is still the easiest and most predictable / understandable way |
Option can be provided for choosing the behaviour in The only problem with alphabetical ordering is refactoring is an issue. Whenever the name changes so that the order changes the FragmentBuilder's arguments need to be changed. And at times this might not even throw an error when both interchange arguments are of the same type. |
Unfortunately that's true. But could also happen if you change the order of your fields during refactoring (but that seems to be more unlikely than renaming a field) Configureable through annotation processing options (project wide) would be an option to specify how the arguments are sorted. |
Sometimes when there 2 or more required arguments, the order of generated builder constructor parameters differs from the order of it's declaration in fragment.
This is specially annoying when Fragment has 2 or more same type args (like String, String, String),
so you forced to build and generate Builder, just to identify arguments order.
The text was updated successfully, but these errors were encountered: