★ wanayoo — archive 1999 https://github.com/nytimes/Store/pull/229Nouvelle recherche | Portail wanayoo
Skip to content
This repository has been archived by the owner. It is now read-only.

/ Store Archived

Add store-kotlin module #229

Merged
merged 11 commits into from Jun 23, 2017
Merged

Add store-kotlin module #229

merged 11 commits into from Jun 23, 2017

Conversation

@stoyicker
Copy link
Contributor

@stoyicker stoyicker commented Jun 13, 2017

This PR adds a new artifact, store-kotlin, which compiles regular Store and includes a few handy methods for Store initialization when using Kotlin.

I doubt this will seamlessly plug onto the current release procedure because the documentation generated by dokka goes somewhere else than the traditional javadoc by default, and I'm not sure what approach to take - could probably use some help with regards to that.
Also, a few tests use reflection via commons lang EqualsBuilder.reflectionEquals(...). If in the future the relevant attributes gain more relaxed access it may be a good idea to edit the tests.
@pavlospt
pavlospt reviewed Jun 13, 2017
View changes
Copy link
Contributor

@pavlospt pavlospt left a comment

Great work! One comment, we could introduce a better DSL for MemoryPolicy I guess since Kotlin allows us to do so :) something like:

memoryPolicy {
    expireAfterWrite = ...
    expireAfterAccess = ...
    expireAfterTimeUnit = ...
    memorySize = ...
}

Let me know what you think :)
store-kotlin/src/main/AndroidManifest.xml Outdated
@@ -0,0 +1,3 @@
<?xml version="1.0" encoding="utf-8"?>
<manifest
package="org.jorge.store.kotlin"/>

This comment has been minimized.

Sign in to view
@pavlospt

pavlospt Jun 13, 2017
Contributor

@stoyicker is this one correct?

This comment has been minimized.

Sign in to view
@stoyicker

stoyicker Jun 14, 2017
Author Contributor

ooopsie
store-kotlin/src/test/kotlin/com/nytimes/android/external/store3/base/impl/MemoryPolicy.kt Outdated
@@ -0,0 +1,9 @@
package com.nytimes.android.external.store3.base.impl

//when(other) {

This comment has been minimized.

Sign in to view
@pavlospt

pavlospt Jun 13, 2017
Contributor

@stoyicker Is this file needed, since it is fully commented?
@digitalbuddha digitalbuddha changed the base branch from tech/store-3-package-rename to feature/rx2 Jun 14, 2017
@digitalbuddha
Copy link
Contributor

@digitalbuddha digitalbuddha commented Jun 14, 2017

your base branch was off I changed it to rx2. As for apache commons I'd prefer for you to take out that dependency as it is an additional dependency we need to manage in future (for only 1 line). Could you also switch to using assertJ fluent assertions. Not using dokka, you can remove that as well. I'll do a full review tonight. Thank you so much for contribution!!
@stoyicker
Copy link
Contributor Author

@stoyicker stoyicker commented Jun 14, 2017 •
edited

First off, thank you guys, always a pleasure to help the project :)
@pavlospt I already included a dsl for MemoryPolicy which works exactly like what you describe, it just needs to be accessed through FluentMemoryPolicyBuilder. The reason for this is consistency: the new builder for Store is called FluentStoreBuilder because the methods in StoreBuilder are static so I can't add their Kotlin parts as extensions functions since static extension functions are not supported. Therefore I made the dsl for MemoryPolicy work the same way, even if technically I could use an extension function for it since MemoryPolicy is not built through static methods but using a builder instance instead.
Now @digitalbuddha your things:

  • Bring in commons is bad, I agree, especially when it's used so little. But I need a way to make value-based comparisons in the tests, and I don't think adding the code for reflection-based comparison myself is a good idea either. What do you think is a good approach here?
  • As far as I know Dokka is not optional, I don't think the Kotlin plugin uses Javadoc at all since running the task on the store-kotlin module does nothing, so removing Dokka is removing all documentation for the module :/
@digitalbuddha
Copy link
Contributor

@digitalbuddha digitalbuddha commented Jun 15, 2017

I think you had a bad rebase, says there are 176 files touched and other commits within the branch.
@pavlospt
Copy link
Contributor

@pavlospt pavlospt commented Jun 15, 2017

@digitalbuddha if the branch that he checked out from to start this was not the Rx2 one, I think the rebase is legit, since it contains all the package name changes.
@stoyicker
Copy link
Contributor Author

@stoyicker stoyicker commented Jun 15, 2017

everything wrt to the base change has been done by github, so now that you guys mention I should probably rebase to drop the commits from the other branch so that the diff correctly matches what this pr is about. will do later today
stoyicker added 5 commits Jun 12, 2017
@stoyicker
Add store-kotlin module
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
eae65b0
@stoyicker
Remove useless file
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
f791658
@stoyicker
Fix package in AndroidManifest
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
a41ac48
@stoyicker
Refactor module tests to assertJ
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
4ab4b25
@stoyicker
Remove wrong annotation
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
Loading status checks…
4a31271
@stoyicker stoyicker force-pushed the stoyicker:kotlin-artifact branch from 4fcfa4e to 4a31271 Jun 15, 2017
stoyicker added 2 commits Jun 15, 2017
@stoyicker
Add store-kotlin artifact to README
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
185cf69
@stoyicker
Add module README
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
Loading status checks…
eb7e3e0
@pavlospt
Copy link
Contributor

@pavlospt pavlospt commented Jun 15, 2017

@stoyicker Impressive work, huge congrats for this one!
@stoyicker
Add KDoc to Javadoc jar
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
Loading status checks…
3f9cf09
@stoyicker
Copy link
Contributor Author

@stoyicker stoyicker commented Jun 15, 2017

thanks :3
I've added a bit of a 'hack' to include kdoc in the javadoc jar so it doesn't go empty to mavencentral, even though it won't be parsable by IDEs. With that, everything other than removing commons lang should now be addressed I think, we just need to decide on that.
@digitalbuddha
Copy link
Contributor

@digitalbuddha digitalbuddha commented Jun 15, 2017

@brianPlummer
Copy link
Contributor

@brianPlummer brianPlummer commented Jun 21, 2017

LGTM! great work!
@brianPlummer
brianPlummer approved these changes Jun 21, 2017
View changes
@digitalbuddha
digitalbuddha reviewed Jun 21, 2017
View changes
...n/src/main/kotlin/com/nytimes/android/external/store3/base/impl/FluentMemoryPolicyBuilder.kt Outdated
/**
* Wraps methods for fluent MemoryPolicy instantiation.
*/
class FluentMemoryPolicyBuilder private constructor() {

This comment has been minimized.

Sign in to view
@digitalbuddha

digitalbuddha Jun 21, 2017
Contributor

lets mark this class with @experimental annotation. you can create that as a new marker interface. This way we can take a few minor revisions to stabilize api prior to shipping
@digitalbuddha
digitalbuddha reviewed Jun 21, 2017
View changes
...tlin/src/main/kotlin/com/nytimes/android/external/store3/base/impl/FluentRealStoreBuilder.kt
* A fluent builder for Store instantiation.
* @param fetcher The fetcher for the created instance.
*/
internal class FluentRealStoreBuilder<Raw, Parsed, Key> constructor(

This comment has been minimized.

Sign in to view
@digitalbuddha

digitalbuddha Jun 21, 2017
Contributor

same mark as @experimental (anything that is public api)
@digitalbuddha
digitalbuddha reviewed Jun 21, 2017
View changes
Copy link
Contributor

@digitalbuddha digitalbuddha left a comment

few annotations and then lgtm
stoyicker added 3 commits Jun 22, 2017
@stoyicker
Add Experimental annotation
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
d748c2c
@stoyicker
Mark all public store-kotlin classes as @experimental
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
fda268e
@stoyicker
Replace use of rx's @experimental with local one instead
Unverified
This user has not uploaded their public key yet.
GPG key ID: 2CA9A539E1438BEC Learn about signing commits
Loading status checks…
88fc76d
@stoyicker
Copy link
Contributor Author

@stoyicker stoyicker commented Jun 22, 2017

thanks! I've added an Experimental annotation of our own and I have also updated usages of rx's one to it. As for the new artifact, all public classes are now annotated as Experimental.
@digitalbuddha digitalbuddha merged commit 10d0cf6 into nytimes:feature/rx2 Jun 23, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@digitalbuddha digitalbuddha mentioned this pull request Jul 5, 2017
Closed
@stoyicker stoyicker deleted the stoyicker:kotlin-artifact branch Aug 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pavlospt pavlospt

@digitalbuddha digitalbuddha

@brianPlummer brianPlummer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@stoyicker @digitalbuddha @pavlospt @brianPlummer
You can’t perform that action at this time.