search

airbnb / DeepLinkDispatch

A simple, annotation-based library for making deep link handling better on Android
http://nerds.airbnb.com/deeplinkdispatch/
4.37k stars 407 forks source link

Made processor 'aggregating' incremental #266

Closed LachlanMcKee closed 4 years ago

LachlanMcKee commented 4 years ago

Hi!

I have managed to make the annotation processor incremental (as suggested via #247)

The main issue that was encountered by @benjamin-bader was that the custom annotations would not work, as you need to be explicit about each annotation that the processor can handle. I believe Qualifiers are different in dagger, as they are used on methods, not classes.

The trick I have used is to use an annotation processor option to allow the user to specify each custom annotation, and then the processor can work as expected.

The obvious downside to this is that users have to remember to do this. I have added some error handling that should remind them.

Thoughts? I don't know if there is a more elegant way to do this.

@adellhk @BenSchwab @elihart @rossbacher

LachlanMcKee commented 4 years ago

This could potentially be an opt-in feature, as you could mark the processor as dynamic, and only make it aggregating if the user decides to specify the annotations in the build.gradle?

LachlanMcKee commented 4 years ago

@elihart I believe I have made the changes that you requested. Please let me know if I missed anything!

@rossbacher @adellhk happy to work with you both to get this across the line :)

LachlanMcKee commented 4 years ago

Hi @elihart, just a heads up - I have made a change to the PR. https://github.com/airbnb/DeepLinkDispatch/pull/266/commits/1a0ffe420f394d85de40aadc7941c9e2f95a4420

I believe it is a very good idea to make the incremental processor opt-in for now. If users upgrade and forget to set the deepLink.customAnnotations, it will fail silently in some circumstances. By making the incremental flag opt in, it would at least encourage them to investigate whether they need to make any more changes.

Thoughts?

elihart commented 4 years ago

Great idea about the opt in, thanks for that change.

LachlanMcKee commented 4 years ago

@elihart what is the process around getting this merged? I think this would be worth a release in itself!

elihart commented 4 years ago

@adellhk @rossbacher Can you approve this and figure out a release plan?

LachlanMcKee commented 4 years ago

In case anyone is interested, I have compiled the binaries on Jitpack (hopefully that's okay). I've tested it out in my current project, and it works a treat :)

com.github.LachlanMcKee.DeepLinkDispatch:deeplinkdispatch:4.3.0-INCREMENTAL
com.github.LachlanMcKee.DeepLinkDispatch:deeplinkdispatch-processor:4.3.0-INCREMENTAL
LachlanMcKee commented 4 years ago

@rossbacher I have addressed your comments

rossbacher commented 4 years ago

Thank you so much for doing this!

condesales commented 4 years ago

any ideas when a new version that includes this will be release?

rossbacher commented 4 years ago

@condesales I just released 5.0.0-beta01 please give it a try. We decided to go beta with this as we totally rewrote the matcher and as it is working totally different we don't know if we covered all existing use cases. It is passing all existing tests, but be are trying to be safe with this.

LachlanMcKee commented 4 years ago

@rossbacher I'm testing 5.1.0 in my companies app, and it appears to be working well! Thanks for getting the release sorted :)