Skip to content
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

Annotation Processor Option for Android's ArrayMap #78

Merged
merged 4 commits into from Oct 31, 2018
Merged

Annotation Processor Option for Android's ArrayMap #78

merged 4 commits into from Oct 31, 2018

Conversation

iNoles
Copy link
Contributor

@iNoles iNoles commented Oct 8, 2017

Fixed #37

@iNoles
Copy link
Contributor Author

iNoles commented Aug 15, 2018

@sockeqwe What is your thought of this?

@sockeqwe
Copy link
Contributor

Sorry, I completely forgot about this PR. Im very sorry!

How would you use this option at the end?

kapt {
   arguments {
      arg ("tikxml.arrayMap", "true")
   }
}

I think we could make this even more generic by passing the full qualified class name of Map implementation that should be used. Something like this:

kapt {
  arguments {
     arg ("tikxml.mapImpl", "android.support.v4.util.ArrayMap")
  }
}

Then we are pretty flexible with any kind of Map implementation the user wants to use, i.e.

arg ("tikxml.mapImpl", "androidx.collection.ArrayMap")

or

arg ("tikxml.mapImpl", "java.util.TreeMap")

The downside, however, is that androidx.collection.ArrayMap is actually not implementing Map interface (android.support.v4.util.ArrayMap does?!?!?)

@WeaponMan
Copy link
Contributor

WeaponMan commented Aug 16, 2018

android.support.v4.util.ArrayMap does implement Map:
public class ArrayMap<K, V> extends SimpleArrayMap<K, V> implements Map<K, V>

@sockeqwe
Copy link
Contributor

Yes, but androidx.collection.ArrayMap is not implementing Map see
https://developer.android.com/reference/kotlin/androidx/collection/ArrayMap

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@512dd48). Click here to learn what that means.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #78   +/-   ##
=========================================
  Coverage          ?   65.45%           
  Complexity        ?      812           
=========================================
  Files             ?      126           
  Lines             ?     3552           
  Branches          ?      841           
=========================================
  Hits              ?     2325           
  Misses            ?      752           
  Partials          ?      475
Impacted Files Coverage Δ Complexity Δ
...va/com/tickaroo/tikxml/processor/XmlProcessor.java 100% <100%> (ø) 14 <0> (?)
...ml/processor/generator/TypeAdapterCodeGenerator.kt 81.96% <44.44%> (ø) 22 <1> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 512dd48...42adc32. Read the comment docs.

@iNoles
Copy link
Contributor Author

iNoles commented Aug 17, 2018

@sockeqwe "this implementation is a version of the platform's android.util.ArrayMap that can be used on older versions of the platform."

https://developer.android.com/reference/androidx/collection/ArrayMap this one said "it is implements Map"

@sockeqwe
Copy link
Contributor

sockeqwe commented Sep 3, 2018

Not sure if we need this. looks like a big switch rather than a hashmap has same / smilar performance characteristics ...

@iNoles
Copy link
Contributor Author

iNoles commented Oct 9, 2018

@sockeqwe close this?

@sockeqwe
Copy link
Contributor

No I will review it on Monday and most likely merge it. Sorry for the long pause and thanks again for your hard work.

@sockeqwe sockeqwe merged commit d2b8bf9 into Tickaroo:master Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants