8000 fix: generic issues in EFragment by Artyomcool · Pull Request #1117 · androidannotations/androidannotations · GitHub
[go: up one dir, main page]

Skip to content
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

Conversation

@Artyomcool
Copy link

Fix for #1077.
It also could affect other generic issues in other enchanced classes because of using the actual class in BundleHelper.

@Artyomcool
Copy link
Author

As always, sorry for being so slow. I wish I had much more time to contribute, because AA is really helps to have a cleaner code.

@WonderCsabo
Copy link
Member

Thanks, really appreciated! Can you look at your previous PR, i added a few
little comments.

@WonderCsabo
Copy link
Member

This PR failed to build in Travis due to ClassCastExceptions while processing, please check.

@Artyomcool
8000
Copy link
Author

Yep, I've forgotten to run functional tests.
I've made a quick fix - just reverted the changes in ServiceActionHandler. But it means, that if we do generic inheritance, like SomeGenericFragment2Ext, there are should be an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Indentation. We use tabs.

@WonderCsabo
Copy link
Member

I started to add indentation comments, but i realized you use spaces everywhere. Can you rebase?

@Artyomcool
Copy link
Author

I could rebase, but not sure where? What commit should I use to rebase on? And how it helps?

@Artyomcool
Copy link
Author

ServiceActionHandler has wrong indents. Should I reformat whole file?

@Artyomcool Artyomcool force-pushed the 1077-fragment-generics branch from de0c1fe to 778485e Compare August 31, 2014 08:28
@Artyomcool
Copy link
Author

Looks like it is out of this PR

@WonderCsabo
Copy link
Member

Yeah, i will reformat that file later. Shoot, we have lots of formatting inconsistencies in this project, i should really create the checkstyle config.

@Artyomcool
Copy link
Author

Yeah, it will help guys like me :)

@WonderCsabo
Copy link
Member

Checkstyle config will be my top priority after we release 3.1, i already started working on it, but it needs serious efforts.

@WonderCsabo
Copy link
Member

@yDelouis I reviewed and tested this PR, and i think this is ready to be merged. I just won't do it because i think it is too late to be in 3.1.

@Artyomcool
Copy link
Author

Are you really sure, that it should be not in 3.1? Ofcource, it is possible, that I've broken something with PR, but without it we have broken functionality for sure.

@WonderCsabo
Copy link
Member

We want to release 3.1 today. Unfortunetaly it seems it will not happen, because @DayS is not responding, and currently he is the only one who has access rights to Maven Central etc.
But if we release today, it is clear that this PR is too late, and we should let it to be tested by SNAPSHOT users. If we postpone 3.1 we can add this feature to it.

@Artyomcool
Copy link
Author

I see. If you are planning to release in a next few days, it is obviously to late to merge this commit.

@WarrenFaith
Copy link

A release of that as a snapshot would be very nice... it is a current blocker on my current project...

Looking forward to 3.1 anyway.
Keep up the good work!

@WonderCsabo
Copy link
Member

If you use Maven, you can easily can build a snapshot from the source. But we are thinking about creating a 3.1 branch, so PRs can be merged the develop and snapshots will be available on OSS.

Copy link
Member

Choose a reason for hiding this comment

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

Tabs instead of spaces!

@WonderCsabo
Copy link
Member

@Artyomcool, unfortunately there is a merge conflict. Also i have found a formatting problem. Can you rebase?

@WonderCsabo
Copy link
Member

@yDelouis i really want to merge this, because this is a serious defect, and effects multiple users. Unfortunately this has conflicts, so we could only merge this with conflict resolution merge commit. It would be much cleaner to rebase this and simply merge.
I thought about rebasing this and push commits to develop. Then we could mark this PR with a label "merged" so we won't forget it in the release notes.

@Artyomcool
Copy link
Author

Give an hour, I'll do the rebase

@WonderCsabo
Copy link
Member

Oh, great, thanks!

BTW @yDelouis, my proposal still holds for other PRs.

@Artyomcool Artyomcool force-pushed the 1077-fragment-generics branch from 778485e to 60fd32d Compare September 28, 2014 18:01
@Artyomcool
Copy link
Author

done

WonderCsabo added a commit that referenced this pull request Sep 28, 2014
@WonderCsabo WonderCsabo merged commit 9713b64 into androidannotations:develop Sep 28, 2014
@WonderCsabo
Copy link
Member

@Artyomcool thanks! You should add the email you are using to your GitHub account, so it would be easier to see who created the commit.

@WonderCsabo WonderCsabo added this to the 3.2 milestone Oct 12, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

0