Initial migration of task backend to typed_sql #8784
Conversation
jonasfj
force-pushed
the
db-task-migration
branch
from
686703f to
06bd9c3
Compare
| List<String> _updatedDependencies( | ||
| List<String>? dependencies, | ||
| List<String>? discoveredDependencies, { | ||
| List<String> _newDependencies( |
There was a problem hiding this comment.
Let's document what this method does.
Aside: This may be in another step, but with SQL, we should probably check if we can get away with simpler behavior, e.g. not reading and processing everything, or, maybe updating the dependent package's timestamp through a query.
| name: 'task', | ||
| as: 'dependencies', | ||
| ) | ||
| abstract final class TaskDependency extends Row { |
There was a problem hiding this comment.
Doesn't this need @PrimaryKey[]?
Also, I'm wondering if the order of the keys should start with the dependency, maybe it could have performance benefits if/when later we update the dependent package's flag/timestamp with a query.
|
|
||
| @immutable | ||
| @JsonSerializable() | ||
| final class TaskState implements CustomDataType<String> { |
There was a problem hiding this comment.
Nit: How far would it be to implement CustomDataType<Json> and using JSONB under the hood? I think it is so wasteful to keep this data as string instead of JSONB...
| await db.tasks | ||
| .where((task) => task.runtimeVersion < gcBeforeRuntimeVersion.asExpr) | ||
| .delete() | ||
| .execute(); |
There was a problem hiding this comment.
Nit: wouldn't the delete() call before the where() make more sense? It kind of make sense for the select() projection to be just before e.g. stream() lie a few lines below, but for delete(), I could imagine it above it. This feels like the delete() and execute() calls should be a single method call, but maybe that fades more with further familiarity of the library use.
| @@ -37,6 +37,11 @@ class _EnvConfig { | |||
| /// Youtube API key to use (skips Datastore secret). | |||
| late final youtubeApiKey = Platform.environment['YOUTUBE_API_KEY']; | |||
|
|
|||
| /// True, if the process is running in a CI environment. | |||
| late final isContinuesIntegration = !['false', '0', ''].contains( | |||
There was a problem hiding this comment.
typo: rename to isContinuousIntegration or isCI?
There was a problem hiding this comment.
though, I think we should rather have the explicit true (or maybe 1) check instead of negating the negative.
No description provided.