Rename QueryData and QueryCache to TargetData and TargetCache #2333

wu-hui · 2019-11-04T17:29:56Z

No description provided.

mikelehen

Some small nits, but overall I'm thrilled with this. Thanks for being thorough (updating comments, etc.). Although it can be tedious to do renames like this, I think making this distinction clear results in a substantial improvement to the understandability of the codebase. Thanks!

mikelehen · 2019-11-04T18:55:24Z

packages/firestore/src/local/local_store.ts

@@ -161,16 +161,16 @@ export class LocalStore {
   */
  private localViewReferences = new ReferenceSet();

-  /** Maps a query to the data about that query. */
-  private queryCache: QueryCache;
+  /** Maps a target to the data about that target query. */


"target query" => "target" ? Or is this intentional?

Changed to use TargetData here.

mikelehen · 2019-11-04T19:03:42Z

packages/firestore/src/local/local_store.ts

        // References for documents sent via Watch are automatically removed
-        // when we delete a query's target data from the reference delegate.
+        // when we delete a target's target data from the reference delegate.


"target's target data" => "target's data"

mikelehen · 2019-11-04T19:12:14Z

packages/firestore/src/local/target_cache.ts


  /**
-   * Adds the given document keys to cached query results of the given target
+   * Adds the given document keys to cached target query results of the given target


nit: "cached target query results" seems a bit awkward. I'd either stick with "cached query results" or simplify to just "cached results"

And do the same for the following 2 diffs...

mikelehen · 2019-11-04T19:13:42Z

packages/firestore/src/local/target_data.ts

@@ -20,59 +20,58 @@ import { Target } from '../core/target';
 import { ListenSequenceNumber, ProtoByteString, TargetId } from '../core/types';
 import { emptyByteString } from '../platform/platform';

-/** An enumeration of the different purposes we have for queries. */
+/** An enumeration of the different purposes we have for targets. */
 export enum QueryPurpose {


Did you consider renaming to TargetPurpose ? I don't feel strongly, but I think it might make sense.

Either way is OK to me, I'll just rename it since you have a slight preference.

mikelehen · 2019-11-04T19:14:32Z

packages/firestore/src/local/target_data.ts

     * LocalStore for user listens and by the SyncEngine for limbo watches.
     */
    readonly targetId: TargetId,
-    /** The purpose of the query. */
+    /** The purpose of the target query. */


Perhaps "target query" => "target" especially if you rename QueryPurpose to TargetPurpose.

mikelehen · 2019-11-04T19:16:24Z

packages/firestore/src/local/target_data.ts

     * was modified.
     */
    readonly sequenceNumber: ListenSequenceNumber,
    /** The latest snapshot version seen for this target. */
    readonly snapshotVersion: SnapshotVersion = SnapshotVersion.MIN,
    /**
-     * The maximum snapshot version at which the associated query view
+     * The maximum snapshot version at which the associated target query view


maybe "target query view" => "view" ?

wu-hui

Thanks for the quick review!

wu-hui · 2019-11-04T19:46:11Z

packages/firestore/src/local/local_store.ts

@@ -161,16 +161,16 @@ export class LocalStore {
   */
  private localViewReferences = new ReferenceSet();

-  /** Maps a query to the data about that query. */
-  private queryCache: QueryCache;
+  /** Maps a target to the data about that target query. */


Changed to use TargetData here.

wu-hui · 2019-11-04T19:47:16Z

packages/firestore/src/local/local_store.ts

        // References for documents sent via Watch are automatically removed
-        // when we delete a query's target data from the reference delegate.
+        // when we delete a target's target data from the reference delegate.


wu-hui · 2019-11-04T19:48:33Z

packages/firestore/src/local/target_cache.ts


  /**
-   * Adds the given document keys to cached query results of the given target
+   * Adds the given document keys to cached target query results of the given target


wu-hui · 2019-11-04T19:51:28Z

packages/firestore/src/local/target_data.ts

     * LocalStore for user listens and by the SyncEngine for limbo watches.
     */
    readonly targetId: TargetId,
-    /** The purpose of the query. */
+    /** The purpose of the target query. */


wu-hui · 2019-11-04T19:52:01Z

packages/firestore/src/local/target_data.ts

@@ -20,59 +20,58 @@ import { Target } from '../core/target';
 import { ListenSequenceNumber, ProtoByteString, TargetId } from '../core/types';
 import { emptyByteString } from '../platform/platform';

-/** An enumeration of the different purposes we have for queries. */
+/** An enumeration of the different purposes we have for targets. */
 export enum QueryPurpose {


Either way is OK to me, I'll just rename it since you have a slight preference.

wu-hui · 2019-11-04T19:52:21Z

packages/firestore/src/local/target_data.ts

     * was modified.
     */
    readonly sequenceNumber: ListenSequenceNumber,
    /** The latest snapshot version seen for this target. */
    readonly snapshotVersion: SnapshotVersion = SnapshotVersion.MIN,
    /**
-     * The maximum snapshot version at which the associated query view
+     * The maximum snapshot version at which the associated target query view


wu-hui added 2 commits November 4, 2019 11:22

QueryCache -> TargetCache

1231cd7

Rename QueryData -> TargetData

b2043cb

wu-hui requested a review from mikelehen November 4, 2019 17:53

wu-hui added the api: firestore label Nov 4, 2019

wu-hui assigned mikelehen Nov 4, 2019

mikelehen approved these changes Nov 4, 2019

View reviewed changes

mikelehen assigned wu-hui and unassigned mikelehen Nov 4, 2019

Rename QueryPurpose -> TargetPurpose

69d58a9

wu-hui commented Nov 4, 2019

View reviewed changes

wu-hui merged commit ed87b76 into master Nov 5, 2019

Feiyang1 added this to the next milestone Nov 5, 2019

firebase locked and limited conversation to collaborators Dec 5, 2019

Rename QueryData and QueryCache to TargetData and TargetCache #2333

Rename QueryData and QueryCache to TargetData and TargetCache #2333

Uh oh!

Conversation

wu-hui commented Nov 4, 2019

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!