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

Firestore adapter lifecycle lint checks #1340

Merged
merged 5 commits into from Jun 12, 2018

Conversation

Projects
None yet
3 participants
@pavlospt
Contributor

pavlospt commented Jun 3, 2018

This PR adds the following Lint checks:

  1. Missing call on startListening() when using a FirestoreRecyclerAdapter.

  2. Missing call on stopListening() when a startListening() method call has occurred previously.

  3. Missing call on setLifecycleOwner() in case FirestoreRecyclerOptions are being used.

Relevant issue: #1339

pavlospt added some commits Jun 3, 2018

Introduce FirestoreRecyclerAdapterLifecycleDetector
This class is responsible to detect:

1) Missing call on startListening() when using a
FirestoreRecyclerAdapter.

2) Missing call on stopListening() when a startListening() method call
has occurred previously.

3) Missing call on setLifecycleOwner() in case FirestoreRecyclerOptions
are being used.
@tasomaniac

Nice tests!

context.getLocation(it.uField),
"Have called .startListening() without .stopListening()."
)
} else if (!it.hasCalledStart) {

This comment has been minimized.

@samtstern

samtstern Jun 6, 2018

Member

Does this trigger if startListening is not called but setLifecycleOwner is called?

@samtstern

samtstern Jun 6, 2018

Member

Does this trigger if startListening is not called but setLifecycleOwner is called?

This comment has been minimized.

@pavlospt

pavlospt Jun 6, 2018

Contributor

Nope have not taken that into account, but I was thinking of it as well. I am not quite sure on the proper approach to solve this.

Currently setLifecycleOwner is applied on FirestoreRecyclerOptions which should be applied on FirestoreRecyclerAdapter, but there is nothing stopping the user of using both that method, as well as, the start/stopListening ones. So I am not quite sure how to prioritize those two together and chose to just add warnings based on the general usage and existence of lifecycle method invocations. That lets the user decide which warning to suppress, effectively acknowledging that it is the desired behavior and not just a method call that they forgot to invoke.

@samtstern I am open to suggestions on the matter if you have anything to suggest :D

Edit 1: I would probably turn the invocation of startListening after setLifecycleOwner has been called, into a RuntimeException so as to give a clear option on the user. Not sure if it should just be a Lint check since in the future those two method calls might conflict in an unpredictable way!

@pavlospt

pavlospt Jun 6, 2018

Contributor

Nope have not taken that into account, but I was thinking of it as well. I am not quite sure on the proper approach to solve this.

Currently setLifecycleOwner is applied on FirestoreRecyclerOptions which should be applied on FirestoreRecyclerAdapter, but there is nothing stopping the user of using both that method, as well as, the start/stopListening ones. So I am not quite sure how to prioritize those two together and chose to just add warnings based on the general usage and existence of lifecycle method invocations. That lets the user decide which warning to suppress, effectively acknowledging that it is the desired behavior and not just a method call that they forgot to invoke.

@samtstern I am open to suggestions on the matter if you have anything to suggest :D

Edit 1: I would probably turn the invocation of startListening after setLifecycleOwner has been called, into a RuntimeException so as to give a clear option on the user. Not sure if it should just be a Lint check since in the future those two method calls might conflict in an unpredictable way!

This comment has been minimized.

@samtstern

samtstern Jun 7, 2018

Member

I think (as @SUPERCILEX mentioned) understanding lifecycle in a lint could be very tricky. Would you be ok with reducing this PR down to just the start/stop calls and not the lifecycle bits?

We'd also have to do the work to ship our custom lint checks, which may be kind of slow, but we don't have to do that in this PR.

@samtstern

samtstern Jun 7, 2018

Member

I think (as @SUPERCILEX mentioned) understanding lifecycle in a lint could be very tricky. Would you be ok with reducing this PR down to just the start/stop calls and not the lifecycle bits?

We'd also have to do the work to ship our custom lint checks, which may be kind of slow, but we don't have to do that in this PR.

This comment has been minimized.

@pavlospt

pavlospt Jun 8, 2018

Contributor

Sure I can do that :)

@pavlospt

pavlospt Jun 8, 2018

Contributor

Sure I can do that :)

This comment has been minimized.

@pavlospt

pavlospt Jun 8, 2018

Contributor

@samtstern I removed setLifecyleOwner related Lint checks and code :)

@pavlospt

pavlospt Jun 8, 2018

Contributor

@samtstern I removed setLifecyleOwner related Lint checks and code :)

@samtstern

This comment has been minimized.

Show comment
Hide comment
@samtstern

samtstern Jun 12, 2018

Member

@pavlospt thanks for making the changes! This looks good for merging into internal and then we'll have to schedule the work of making a public lint module.

Member

samtstern commented Jun 12, 2018

@pavlospt thanks for making the changes! This looks good for merging into internal and then we'll have to schedule the work of making a public lint module.

@samtstern samtstern merged commit d460977 into firebase:version-4.1.0-dev Jun 12, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pavlospt

This comment has been minimized.

Show comment
Hide comment
@pavlospt

pavlospt Jun 12, 2018

Contributor

Glad to see so :D Thanks @samtstern !

Contributor

pavlospt commented Jun 12, 2018

Glad to see so :D Thanks @samtstern !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment