Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

fix issue issues with generic @EView and @EViewGroup#1075

Merged
WonderCsabo merged 1 commit into
androidannotations:developandroidannotations/androidannotations:developfrom
Artyomcool:1056-eviewgroup-genericsArtyomcool/androidannotations:1056-eviewgroup-genericsCopy head branch name to clipboard
Sep 20, 2014
Merged

fix issue issues with generic @EView and @EViewGroup#1075
WonderCsabo merged 1 commit into
androidannotations:developandroidannotations/androidannotations:developfrom
Artyomcool:1056-eviewgroup-genericsArtyomcool/androidannotations:1056-eviewgroup-genericsCopy head branch name to clipboard

Conversation

@Artyomcool

Copy link
Copy Markdown

There are still some issues with generics corner cases like A super B, or A extends B&C. If issues like A extends B&C just time consuming, in case of A super B it looks like there is no way to handle it. This is because JMethod's generify creates JTypeVar, that final and always produces bounds with "extends" keyword. But if something comes to your minds, guys, please, share your ideas :)

@WonderCsabo

Copy link
Copy Markdown
Member

Thanks for this! BTW, all the other enhanced components can work with generics?

@Artyomcool

Copy link
Copy Markdown
Author

I checked code for the same issues and didn't find them. But it is possible
that there are another problems with generics, especially in fragments and
activities. Maybe we just need to write tests to figure it out )))

@Artyomcool

Copy link
Copy Markdown
Author

Right. Fragments, Activities and some other components have generated
builders. Good place for generics issues.

@WonderCsabo

Copy link
Copy Markdown
Member

I just tried it out:

  • Activity: There are no compilation problems, the generated class is generic.
  • Fragment: The class is OK as with Activities, but the FragmentBuilder is totally messed up.
  • Service: OK.

@Artyomcool

Copy link
Copy Markdown
Author

OK. Let's create another issue for fragments. I'll try to find the time to
fix it.
29 июля 2014 г. 2:24 пользователь "Csaba Kozák" notifications@github.com
написал:

I just tried it out:

  • Activity: There is no compilation problems, the generated class is
    generic.
  • Fragment: The class is OK as with Activities, but the
    FragmentBuilder is totally messed up.
  • Service: OK.


Reply to this email directly or view it on GitHub
#1075 (comment)
.

@WonderCsabo

Copy link
Copy Markdown
Member

Thanks @Artyomcool, really appreciated! I created #1077.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should match the method name and the parameter name (helper vs factory)

@WonderCsabo

Copy link
Copy Markdown
Member

You are using space indentation instead of tabs. Please use the formatting settings provided with the projects.

@WonderCsabo

Copy link
Copy Markdown
Member

Multiple files are missing the license header. Please add them by executing mvn license:format in the parent project.

@Artyomcool

Copy link
Copy Markdown
Author

@WonderCsabo, I've added a license headers here, and in #1117 for classes created by me. As for tabs and spaces there is a problem. I'm using an Intellij Idea as IDE, so, your settings file can't be used directly. If I'll just reformat hole code with tabs, I'm pretty sure, that I'll break the rest formating, like imports order and so on. Could you provide settings for Intellij Idea? Or maybe just do it for me? In this case, I'll be more carefull next time.

@WonderCsabo

Copy link
Copy Markdown
Member

Yeah, we already had problems about IDEA, unfortunetaly importing the eclipse prefs do not work. I want to add checkstyle to the build so at least the build fails with basic errors in formatting.

About rebasing: i do not like commits like "reformat code", this looks really dumb in commit history. Your formatting is ok, so if you can just replace spaces with tabs and squash this into a single commit, that would be great. The same applies to the other PR.

@Artyomcool

Copy link
Copy Markdown
Author

Ok, I'll do it by reset --mixed. In this way I'll be able to see, what code was changed and reformat it localy.

@WonderCsabo

Copy link
Copy Markdown
Member

Thanks. Please note that i would do that work for you but i cannot merge modified commits by me, because in that case GH does not marks the PR as merged, and we list contributors and changes in release notes from merged PRs.

@Artyomcool

Copy link
Copy Markdown
Author

I think, the best way to resolve code style issue - is to make me do it :) It is not because of contributors list - I'm actually don't care about. But it is the only way to remember, that I should respect and follow your code style.

@Artyomcool Artyomcool force-pushed the 1056-eviewgroup-generics branch from 1bc06b3 to dd6987d Compare August 31, 2014 07:53
@WonderCsabo

Copy link
Copy Markdown
Member

Thanks. I am sorry, but i have another request, which is in contrast to my previous one. I did not know that you have wrong formatting in the other files, it seems @DayS did not recognized when he reviewed. Can i ask you to separate these formattings to another commit, since these has nothing to do with generic @EViewGroup?

@WonderCsabo

Copy link
Copy Markdown
Member

Can you add a compile-time test with generic ViewGroup?

Edit: I just realized that you had tests in your previous commit. It seems you unintentionally removed those. I still have them in my local repo, so i can push them to be available, but i think you can also find those in your reflog.

@Artyomcool

Copy link
Copy Markdown
Author

Could you pin point, where should I fix the formatting?

@WonderCsabo

Copy link
Copy Markdown
Member

Maybe my request was not clear. You have changed SupposeBackgroundHandler, SupposeThreadHandler and SupposeUiThreadHandler in commit dd6987d. This commit is about fixing generic issues in ViewGroups, so the formatting changes you made should be in a separate commit.

I know i said i do not like commits like "formatting", but in this case we cannot do anything else, since the wrong formatted code is already pushed to develop.

@Artyomcool

Copy link
Copy Markdown
Author

If someone will face the same problem with indentation, I could share my experience. It can be easely fixed step-by-step for IDEA:

  1. File->Settings
  2. Code Style->Java->Tabs and Indents
  3. Check "Use tab character"
  4. Use git reset --mixed HEADN, where N is number of commits to fix and squashm e.g. HEAD1
  5. Commit through IDE: Right click on the project -> Git -> Commit directory...
  6. Check "Reformat code"
  7. Commit
  8. Push with --force option

It will reformat only changed code, so it should not break the rest of formatitng.

@Artyomcool Artyomcool force-pushed the 1056-eviewgroup-generics branch from dd6987d to 19318da Compare August 31, 2014 08:15
@Artyomcool

Copy link
Copy Markdown
Author

Repushed without reformating the SupposeThread feature.

@Artyomcool

Copy link
Copy Markdown
Author

I'll add requested tests a little bit later, I need some time to fix all issues with formating :)

@WonderCsabo

Copy link
Copy Markdown
Member

Open another PR for reformatting the other files, then, so we are really clear with intents of PRs and commits.

@Artyomcool

Copy link
Copy Markdown
Author

Right.

@Artyomcool

Copy link
Copy Markdown
Author

Done: #1119

@Artyomcool

Copy link
Copy Markdown
Author

As for tests - could you add it from your local repo? It will be much faster, then searching through reflog.

@WonderCsabo

Copy link
Copy Markdown
Member

You can find those in this branch. Actually i refetched your repo so i had to search my reflog. :)

@WonderCsabo

Copy link
Copy Markdown
Member

BTW, there are some updates in the instruction for code contributors, please read them.

…rror there is a APTCodeModelHelper.typeMirrorToJClass. It handles several corner cases with generics, while ProcessHolder.refClass - don't.

fix: factories in generated ViewGroup's now correctly generified
@Artyomcool Artyomcool force-pushed the 1056-eviewgroup-generics branch from 19318da to f5f9742 Compare August 31, 2014 09:14
@Artyomcool

Copy link
Copy Markdown
Author

I've done a lot of resets today, so it was tricky to find the tests. I still don't know when I lost them, but now the are restored and everything squashed into a single commit.

@WonderCsabo

Copy link
Copy Markdown
Member

@dodgex Thanks for all your work and for doing all my annoying requests! You did a great job on generics handling in AA.

@yDelouis This PR is ready to be merged, but it will miss 3.1 if we release that very soon.

@dodgex

dodgex commented Aug 31, 2014

Copy link
Copy Markdown
Member

@WonderCsabo while i'm happy that you are thanking me, i think you should thank @Artyomcool in this PR ;)

@Artyomcool

Copy link
Copy Markdown
Author

@WonderCsabo, @dodgex also did a great work for AA, but not generic issues ;)

@WonderCsabo

Copy link
Copy Markdown
Member

Sorry. It seems i should do less things in parallel... So thanks, @Artyomcool for the generic stuff. :)

@Artyomcool

Copy link
Copy Markdown
Author

And thank you, guys, for a great framework.
BTW: somewhere should be a place, where contributers could peek an issue to do next, what do you think?

@dodgex

dodgex commented Aug 31, 2014

Copy link
Copy Markdown
Member

@Artyomcool i think we can agree that we both (and all other contributors) did great work. ;) its work worth it, as AA takes the work from us on other places. :)

@Artyomcool try this or this or simply this ;) at least thats what i did. :p

@WonderCsabo

Copy link
Copy Markdown
Member

contributers could peek an issue to do next

You mean "pick" an issue? In the past @DayS added the milestones for every proposed feature which has been accepted to be a good one. So you can look at those lists what @dodgex referenced. We want to change the release model of AA to get more frequent releases and maybe more motivation to do the features, so perhaps these milestones will be dropped. Since this is not related to this PR, we should really talk about this in the mailing list.

@WonderCsabo WonderCsabo changed the title fix issue #1056 fix issue issues with generic @EView and @EViewGroup Sep 1, 2014
WonderCsabo added a commit that referenced this pull request Sep 20, 2014
@WonderCsabo WonderCsabo merged commit 3512756 into androidannotations:develop Sep 20, 2014
@WonderCsabo

Copy link
Copy Markdown
Member

Finally merged. Thanks.

@yDelouis yDelouis added this to the 3.2 milestone Sep 21, 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.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.