Skip to content
This repository has been archived by the owner on Jun 18, 2020. It is now read-only.

Deprecate XingApi's resource function #245

Merged
merged 4 commits into from
Jul 31, 2019
Merged

Deprecate XingApi's resource function #245

merged 4 commits into from
Jul 31, 2019

Conversation

aballano
Copy link
Contributor

@aballano aballano commented Jul 4, 2019

Deprecate XingApi's resource function as it might use reflection if no factories are declared, which causes problems with proguard unnecessarily.

* @deprecated This function might use reflection if the factory is not provided,
* better pass this object to the resource to be created instead.
*/
@Deprecated @SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an alternative solution already then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm not mistaken this is used in order to cache Resources inside this class, right? If so, an alternative would be to simply use dagger to cache what needs to be cached instead of having this implicit behaviour here.

Maybe you remember better if this function had any other benefit apart from that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yeah, I see your point now.

Although, I believe that is an important change on how library works. Maybe a little bit more explanation about it would be better. Because I believe the original idea was to handle caching as part of library.

  • If the plan to remove this functionality (which means removing the resource caching all together) maybe mention that.
  • If it is only about to deprecate, and then maybe explain how would be the better approach - for documentation purposes. I assume by this change, you mean instead of getting the resource instance as xingApi.resource(WhateverResource.class) should be created as WhateverResource(xingApi) and caching should be handled by developer themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also you mentioned as "it might use reflection if factory is not provided" - while I understand that can be a hidden cost, it is also a design decision, isn't it? You might wanna change that part instead of deprecating the caching mechanism then 🤷‍♂

How about throwing an IllegalStateException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, we've opted for removing this completely, it will be a breaking change but we consider it important in order to prevent issues with Proguard in the future.

Also removed factories and made all resource's constructors public.
Copy link
Collaborator

@MyDogTom MyDogTom left a comment

Choose a reason for hiding this comment

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

Thanks for the change! I have a question regarding deployment and a small suggestion.

.buildscript/deploy_version.sh Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Remove old travis step
@aballano aballano force-pushed the aballano-patch-1 branch 3 times, most recently from b069ce4 to f01c735 Compare July 31, 2019 11:24
@aballano aballano merged commit 7f2314a into master Jul 31, 2019
@aballano aballano deleted the aballano-patch-1 branch July 31, 2019 12:30
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