Expose DatabaseRegion(table:) initializer

[charlesmchen-signal]

Hi there,

We'd like to use DatabaseRegion and DatabaseRegionObservation to observe changes to db tables. To do so, we need the DatabaseRegion(table:) initializer to be public.

[groue]

Hello @charlesmchen-signal. Wow, Signal is pretty involved here these days, it's a pleasure to see you all :-)

So you want to make DatabaseRegion(table:) public. OK, I don't really have anything against.

Is it possible that you complete the PR checklist from the pull request template?

• This pull request is submitted against the development branch.

  Since we're aiming for GRDB 4 now, please target the GRDB-4.0 branch instead.

• Inline documentation has been updated.

  I think the current inline doc was OK for an internal method, but not for a public method.

• README.md or another dedicated guide has been updated.

  You can skip this one.

• Changes are tested, for all flavors of GRDB, GRDBCipher, and GRDBCustomSQLite.

  You can skip this one: DatabaseRegionTests already tests DatabaseRegion(table:).

[charlesmchen-signal]

Hi @groue - Thanks for your work on GRDB.

Pull Request Checklist

• This pull request is submitted against the development branch.

Since we're aiming for GRDB 4 now, please target the GRDB-4.0 branch instead.

Done.

• [?] Inline documentation has been updated.

I think the current inline doc was OK for an internal method, but not for a public method.

I've elaborated the docs - I'm not sure what you consider sufficient. I'd be happy to go further if you'd be kind enough to offer some guidance.

While doing this, I decided to go ahead and elaborate the comments on the other initializers and make them public as well.

• README.md or another dedicated guide has been updated.

You can skip this one.

Skipped.

• Changes are tested, for all flavors of GRDB, GRDBCipher, and GRDBCustomSQLite.

You can skip this one: DatabaseRegionTests already tests DatabaseRegion(table:).

Skipped.

[groue]

Oops, things have turned wild. I guess it's my fault. I'll try to provide better guidance.

First of all, no DatabaseRegion initializer was public. This is because DatabaseRegion is tightly coupled with SQLite inner guts, and a few GRDB optimizations which are supposed to remain hidden implementation details.

We're dealing with a key GRDB feature here, which is the definition of observed regions. As you have noticed, besides the trivial empty DatabaseRegion() and DatabaseRegion.fullDatabase, the only currently available public way to define a region is through a request:

DatabaseRegionObservation(tracking: Player.all())
DatabaseRegionObservation(tracking: Player.filter(key: 1))
DatabaseRegionObservation(tracking: SQLRequest<Player>(sql: "SELECT * FROM player"))

I think it is good to define a region through a request, because 1. this makes very clear in application code what results the application is interested in, 2. it grants GRDB opportunities to add optimizations for certain kinds of requests, which means that your application becomes more efficient as new GRDB versions ship, 3. it provides a unified way to define regions, from trivial requests to the most complex ones.

Learn once, apply everywhere.

Conclusion: I want that requests remain the one true fostered way to define regions.

Now, it looks like your request for a public DatabaseRegion(table:) goes against this goal. It does, but it also is harmless. A region defined as a full table is a simple concept that does not require any kind of explanation or optimization. And maybe you don't feel like defining a record type for your database table. So you're good to go, and I accepted the pitch:

// The request way
struct Player: TableRecord { }
DatabaseRegionObservation(tracking: Player.all())

// This pull request
DatabaseRegionObservation(tracking: DatabaseRegion(table: "player"))

Other initializers are, on the other side, really much too much precise and implementation-dependent, and I prefer to keep them private:

// The request way only
DatabaseRegionObservation(tracking: Player.select(Column("name")))
DatabaseRegionObservation(tracking: Player.filter(keys: [1, 2, 3]))

---

So :-) If the "request way" still does not convince you, and you still need DatabaseRegion(table:), let's move forward:

Please keep only DatabaseRegion(table:) public, and revert other changes.

For the documentation guidance, I suggest you have a look at API Design Guidelines . It talks about documentation comments. This could give:

/// Creates a region that spans all rows and columns of a database table.
///
/// - parameter table: A table name.
 init(table: String) { ... )

[groue]

First of all, no DatabaseRegion initializer was public. This is because DatabaseRegion is tightly coupled with SQLite inner guts, and a few GRDB optimizations which are supposed to remain hidden implementation details.

In all honesty, this wasn't easy to guess for you 😅. I was so happy with the DatabaseRegion type that I bragged about its implementation in the documentation, instead of remaining on the very edge of what's useful with it. I guess this is a classical sin, a common mistake: writing doc is hard, especially when you're the one who wrote the code. Eventually, the documentation for DatabaseRegion will be enhanced. And today, let's just pretend we don't know how it works 😈

[charlesmchen-signal]

Hi @groue,

I've pushed a commit that takes your suggestions.

Thanks

[groue]

Perfect, @charlesmchen-signal, thank you!

I'll push a couple other commits and merge your PR :-)

[charlesmchen-signal]

Thank you @groue

[groue]

Hello @charlesmchen-signal,

I intend to deprecate DatabaseRegion(table:), since GRDB 5.15 now has a Table type that conforms to DatabaseRegionConvertible.

For example:

-DatabaseRegionObservation(tracking: DatabaseRegion(table: "player"))
+DatabaseRegionObservation(tracking: Table("player"))

(Table is not only about building regions, it is also about using the query builder without any record type, see Requests.)

Would this deprecation, and replacement solution, fit your needs at Signal?

cc @michaelkirk-signal

[groue]

OK, so DatabaseRegion(table:) will be deprecated in next version, and removed in GRDB 6.