-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
🔨 add types for database rows and prepare use of knex instead of TypeORM #3064
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
51d723c
to
1d68047
Compare
b457fa3
to
3862ef8
Compare
168bd71
to
a5ba090
Compare
a5ba090
to
f3522c9
Compare
163232b
to
ea567fe
Compare
0c64f41
to
0ee6c51
Compare
Warning Rate Limit Exceeded@danyx23 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 47 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates focus on refining the data model and enhancing type specificity across various components of the system. Key changes include the introduction of Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
db/sql-ts/sql-ts-config.json
is excluded by:!**/*.json
package.json
is excluded by:!**/*.json
yarn.lock
is excluded by:!**/*.lock
Files selected for processing (45)
- adminSiteServer/apiRouter.ts (2 hunks)
- baker/algolia/indexChartsToAlgolia.ts (2 hunks)
- db/model/Link.ts (1 hunks)
- db/sql-ts/README.md (1 hunks)
- db/sql-ts/template.handlebars (1 hunks)
- db/tests/basic.test.ts (1 hunks)
- db/wpdb.ts (2 hunks)
- packages/@ourworldindata/components/src/GdocsUtils.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Charts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Entities.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Images.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Origins.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Posts.ts (2 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sources.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Tags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Users.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Variables.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Tag.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Various.ts (1 hunks)
- packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts (3 hunks)
- packages/@ourworldindata/types/src/index.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- db/sql-ts/template.handlebars
Additional comments: 44
packages/@ourworldindata/types/src/domainTypes/Tag.ts (1)
- 1-10: The
Tag
interface is simple and straightforward, defining properties for a tag entity. All fields are required, which implies that when creating aTag
, all these values must be provided. This is a reasonable expectation for a tag entity.packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts (1)
- 1-8: The
AnalyticsPageviewsRow
interface correctly defines the structure for an analytics pageviews database row with all required fields. The naming convention is consistent, and the use ofDate
for theday
field is appropriate for representing a specific day.packages/@ourworldindata/types/src/dbTypes/Sessions.ts (1)
- 1-7: The
SessionsRowForInsert
interface and theSessionsRow
type are well-defined. TheSessionsRow
type uses theRequired
utility type to ensure all properties fromSessionsRowForInsert
are required, which is a common pattern for distinguishing between create and read DTOs (Data Transfer Objects).packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts (1)
- 1-6: The
PostsGdocsXTagsRowForInsert
interface and thePostsGdocsXTagsRow
type are correctly defined. TheRequired
utility type is used to make all properties required for thePostsGdocsXTagsRow
type, which is a standard approach for ensuring data integrity when reading from the database.packages/@ourworldindata/types/src/dbTypes/PostTags.ts (1)
- 1-8: The
PostTagsRowForInsert
interface introduces optional fields forcreatedAt
andupdatedAt
, which is typical for insert operations where these fields are often managed by the database. ThePostTagsRow
type then correctly uses theRequired
utility type to enforce that all properties must be present when working with an existing row.packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts (1)
- 1-7: The
ExplorerChartsRowForInsert
interface is defined with an optionalid
field, which is common for auto-incremented primary keys. TheExplorerChartsRow
type then makes all fields required, which is appropriate for reading existing rows where theid
would be known.packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts (1)
- 1-7: The
PostsGdocsXImagesRowForInsert
interface and thePostsGdocsXImagesRow
type are correctly structured. The optionalid
field in the insert interface suggests that theid
is auto-generated, and theRequired
utility type is used to ensure all properties are present in thePostsGdocsXImagesRow
type.packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts (1)
- 1-8: The
DatasetTagsRowForInsert
interface is well-defined with optionalcreatedAt
andupdatedAt
fields, which is standard for insert operations. TheDatasetTagsRow
type uses theRequired
utility type to make all properties required, ensuring that all necessary data is present when working with existing rows.packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts (1)
- 1-7: The
ExplorerVariablesRowForInsert
interface is defined with an optionalid
field, indicating that theid
is likely auto-generated by the database. TheExplorerVariablesRow
type correctly uses theRequired
utility type to ensure all properties are present for existing rows.packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts (1)
- 1-7: The
OriginsVariablesRowForInsert
interface includes an optionaldisplayOrder
field, which is reasonable for cases where the order might not be specified during insertion. TheOriginsVariablesRow
type then makes all fields required, which is appropriate for ensuring complete data when reading from the database.packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts (1)
- 1-9: The
DatasetFilesRowForInsert
interface is well-structured with optionalcreatedAt
andupdatedAt
fields, which aligns with common database practices where these timestamps are managed automatically. TheDatasetFilesRow
type ensures all properties are required, which is suitable for reading existing rows.packages/@ourworldindata/types/src/dbTypes/Images.ts (1)
- 1-10: The
ImagesRowForInsert
interface is defined with several optional fields, includingid
,originalWidth
, andupdatedAt
, which is typical for database operations where some fields may not be immediately known or are auto-generated. TheImagesRow
type uses theRequired
utility type to ensure all properties are present for existing rows.packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts (1)
- 1-8: The
TagsVariablesTopicTagsRowForInsert
interface and the correspondingTagsVariablesTopicTagsRow
type are correctly defined. The optionaldisplayOrder
field in the insert interface suggests that not all tags will have a specified order, and theRequired
utility type is used to ensure all properties are present in theTagsVariablesTopicTagsRow
type.packages/@ourworldindata/types/src/dbTypes/Namespaces.ts (1)
- 1-10: The
NamespacesRowForInsert
interface is well-defined with several optional fields, which is appropriate for insert operations where not all information may be available. TheNamespacesRow
type then makes all fields required, which is standard for ensuring data completeness when reading existing rows.packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts (1)
- 1-9: The
ChartSlugRedirectsRowForInsert
interface is structured with optional fields forcreatedAt
,id
, andupdatedAt
, which is typical for database operations where these fields are often managed by the database itself. TheChartSlugRedirectsRow
type ensures all properties are required for existing rows.packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts (1)
- 1-8: The
CountryLatestDataRowForInsert
interface is defined with all optional fields, which suggests flexibility in the data that can be inserted. TheCountryLatestDataRow
type uses theRequired
utility type to enforce that all properties must be present when working with an existing row, which is a common pattern for read operations.packages/@ourworldindata/types/src/dbTypes/Entities.ts (1)
- 1-11: The
EntitiesRowForInsert
interface is well-structured with optional fields forcode
,createdAt
,id
,updatedAt
, andvalidated
. TheEntitiesRow
type then makes all fields required, which is appropriate for ensuring data integrity when reading existing rows.packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts (1)
- 1-9: The
PostsGdocsVariablesFaqsRowForInsert
interface and the correspondingPostsGdocsVariablesFaqsRow
type are correctly defined. The optionaldisplayOrder
field in the insert interface suggests that not all FAQs will have a specified order, and theRequired
utility type is used to ensure all properties are present in thePostsGdocsVariablesFaqsRow
type.packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts (1)
- 1-11: The
ChartDimensionsRowForInsert
interface is defined with optional fields forcreatedAt
,id
, andupdatedAt
, which is typical for database operations where these fields are often managed by the database itself. TheChartDimensionsRow
type ensures all properties are required for existing rows.packages/@ourworldindata/types/src/dbTypes/Tags.ts (1)
- 2-13: The
TagsRowForInsert
interface has been updated to include optional fields, which is appropriate for insert operations where not all information may be available. TheTagsRow
type then makes all fields required, which is standard for ensuring data completeness when reading existing rows. The addition of theTagsRowTableName
constant provides a clear reference to the table name, which is a good practice for maintainability.packages/@ourworldindata/types/src/dbTypes/Users.ts (1)
- 1-14: The
UsersRowForInsert
interface is well-structured with optional fields forcreatedAt
,isActive
,isSuperuser
,lastLogin
,lastSeen
,password
, andupdatedAt
, which is typical for user-related operations where some fields may be managed by the system or not required at the time of insertion. TheUsersRow
type uses theRequired
utility type to ensure all properties are present for existing rows.packages/@ourworldindata/types/src/dbTypes/Explorers.ts (1)
- 1-12: The
ExplorersRowForInsert
interface is defined with optional fields forcreatedAt
andupdatedAt
, which is common for database operations where these timestamps are managed automatically. TheExplorersRow
type ensures all properties are required, which is suitable for reading existing rows. TheTODO
comment on line 12 indicates a future improvement to add an enriched type and properly type the config, which is a good reminder for maintainability and code quality.packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts (1)
- 1-14: The
PostsLinksRowForInsert
interface is well-defined with a mix of required and optional fields. The optionalid
andlinkType
fields suggest that theid
is auto-generated and thelinkType
may not always be known at insertion. ThePostsLinksRow
type correctly uses theRequired
utility type to ensure all properties are present for existing rows.packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts (1)
- 1-14: The
PostsGdocsLinksRowForInsert
interface is defined with a mix of required and optional fields, including an optionalid
andsourceId
, which is typical for database operations where some fields may be auto-generated or not required at the time of insertion. ThePostsGdocsLinksRow
type uses theRequired
utility type to ensure all properties are present for existing rows.packages/@ourworldindata/types/src/dbTypes/ChartTags.ts (1)
- 4-20: The
ChartTagsRowForInsert
interface is well-structured with optional fields forcreatedAt
,isApproved
,keyChartLevel
, andupdatedAt
, which is typical for database operations where some fields may be managed by the database itself or not required at the time of insertion. TheChartTagsRow
type ensures all properties are required for existing rows. TheChartTagJoin
type on lines 19-20 is a minimal union of the tags and chart_tags entities, which is useful for components that require a combination of these entities.packages/@ourworldindata/types/src/dbTypes/Datasets.ts (1)
- 1-22: The
DatasetsRowForInsert
interface is defined with several optional fields, which is appropriate for insert operations where not all information may be available. TheDatasetsRow
type uses theRequired
utility type to enforce that all properties must be present when working with an existing row, which is a common pattern for read operations.db/sql-ts/README.md (1)
- 1-9: The README provides clear instructions on how to use the SQL-TS tool to generate TypeScript types from a MySQL database. It explains the outcome of running the tool and where to find the configuration and template files. The instructions for running the tool are concise and straightforward.
packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts (1)
- 5-32: The
ChartRevisionsRowForInsert
interface is well-defined with optional fields forchartId
,config
,createdAt
,id
,updatedAt
, anduserId
, which is typical for database operations where some fields may be managed by the database itself or not required at the time of insertion. TheChartRevisionsRowRaw
andChartRevisionsRowEnriched
types are correctly structured to ensure all properties are present for existing rows and to enrich theconfig
field with a parsedGrapherInterface
. The functionsparseChartRevisionsRow
andserializeChartRevisionsRow
are provided to handle the conversion between raw and enriched rows, which is a good practice for maintainability and data integrity.packages/@ourworldindata/types/src/dbTypes/Origins.ts (1)
- 4-33: The
OriginsRowForInsert
interface is well-structured with several optional fields, which is appropriate for insert operations where not all information may be available. TheOriginsRowRaw
andOriginsRowEnriched
types are correctly structured to ensure all properties are present for existing rows and to enrich thelicense
field with a parsedLicense
. The functionsparseOriginsRow
andserializeOriginsRow
are provided to handle the conversion between raw and enriched rows, which is a good practice for maintainability and data integrity.packages/@ourworldindata/components/src/GdocsUtils.ts (1)
- 1-10: The
getLinkType
function has been updated to use theOwidGdocLinkType
enum, which is a good practice for type safety and code clarity. The function now returns an enum value instead of a string, which can help prevent bugs related to incorrect link type values.packages/@ourworldindata/types/src/dbTypes/Charts.ts (1)
- 4-42: The
ChartsRowForInsert
interface is well-defined with optional fields forcreatedAt
,id
,is_indexable
,isExplorable
,publishedAt
,publishedByUserId
,slug
,type
, andupdatedAt
, which is typical for database operations where some fields may be managed by the database itself or not required at the time of insertion. TheChartsRowRaw
andChartsRowEnriched
types are correctly structured to ensure all properties are present for existing rows and to enrich theconfig
field with a parsedGrapherInterface
. The functionsparseChartConfig
,serializeChartConfig
,parseChartsRow
, andserializeChartsRow
are provided to handle the conversion between raw and enriched rows, which is a good practice for maintainability and data integrity.packages/@ourworldindata/types/src/dbTypes/Sources.ts (1)
- 11-45: The
SourcesRowForInsert
interface is well-structured with optional fields forcreatedAt
,datasetId
,id
, andupdatedAt
, which is typical for database operations where some fields may be managed by the database itself or not required at the time of insertion. TheSourcesRowRaw
andSourcesRowEnriched
types are correctly structured to ensure all properties are present for existing rows and to enrich thedescription
field with a parsedSourceDescription
. The functionsparseSourceDescription
,serializeSourceDescription
,parseSourcesRow
, andserializeSourcesRow
are provided to handle the conversion between raw and enriched rows, which is a good practice for maintainability and data integrity.packages/@ourworldindata/types/src/domainTypes/Various.ts (1)
- 5-5: The introduction of the
JsonString
type is a good practice for clarifying the intent of string values that are expected to contain JSON content. It enhances readability and maintainability by making the expected content of the string explicit.packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts (4)
- 1-1: The import path for
Tag
has been updated to reflect the new location of theTag
type definition. Ensure that the new path is correct and that theTag
type is indeed located within"../domainTypes/Tag.js"
.- 101-101: A new type
OwidGdocContent
has been added. Ensure that this type is being used where appropriate and that it integrates well with the existing types.- 133-138: The introduction of the
OwidGdocLinkType
enum is a good practice as it replaces hard-coded string literals with a defined set of values, which can help prevent typos and make the codebase more maintainable.- 142-142: The
linkType
property in theOwidGdocLinkJSON
interface now uses theOwidGdocLinkType
enum. This change enhances type safety and should be reflected whereverOwidGdocLinkJSON
is used.packages/@ourworldindata/types/src/dbTypes/Variables.ts (1)
- 1-220: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-608]
The file
Variables.ts
defines interfaces and functions for working with variables, including serialization and deserialization of various JSON fields. Ensure that all the functions handle potential exceptions that may occur during JSON parsing and that they are used consistently throughout the codebase.Verification successful
The review comment is verified for the
parseVariableDisplayConfig
function, which does not include exception handling for JSON parsing. It is recommended to implement exception handling for this and potentially other serialization and deserialization functions to ensure robustness.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the functions are used consistently and handle exceptions rg "parseVariableDisplayConfig|serializeVariableDisplayConfig|parseVariableDimensions|serializeVariableDimensions|parseVariablesRow|serializeVariablesRow|parseVariableDescriptionKey|serializeVariableDescriptionKey|parseVariableOriginalMetadata|serializeVariableOriginalMetadata|parseVariableLicenses|serializeVariableLicenses|parseVariableGrapherConfigAdmin|serializeVariableGrapherConfigAdmin|parseVariableGrapherConfigETL|serializeVariableGrapherConfigETL|parseVariableProcessingLog|serializeVariableProcessingLog"Length of output: 5780
Script:
#!/bin/bash # Inspect the implementation of the parseVariableDisplayConfig function for exception handling rg "parseVariableDisplayConfig" --context 10 packages/@ourworldindata/types/src/dbTypes/Variables.tsLength of output: 1623
packages/@ourworldindata/types/src/index.ts (2)
- 52-52: The export of
Tag
has been updated to reflect its new location. This change should be cross-referenced with other files to ensure that they importTag
from the correct location.- 385-608: A significant number of new exports related to database types have been added. Ensure that these new exports are being used appropriately and that they do not conflict with existing types or logic in the codebase.
db/wpdb.ts (2)
- 44-44: The import statement has been updated to include
OwidGdocLinkType
. This change is consistent with the PR's objective to use TypeScript enums for type safety.- 825-825: The string literal "grapher" has been correctly replaced with
OwidGdocLinkType.Grapher
in thegetPublishedLinksTo
function call. This change enhances type safety and is in line with the PR's objectives.adminSiteServer/apiRouter.ts (2)
- 53-57: The import statement for
OwidGdocLinkType
has been added, which is consistent with the summary indicating the use of this new enum in the code.- 228-228: The string literal
'Grapher'
has been replaced withOwidGdocLinkType.Grapher
. This change aligns with the summary's mention of replacing string literals with enums to enhance type safety and clarity.
@@ -278,6 +276,7 @@ export { | |||
GdocsContentSource, | |||
type OwidArticleBackportingStatistics, | |||
type LinkedChart, | |||
OwidGdocLinkType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The new context provided shows that OwidGdocLinkType
is indeed used within db/wpdb.ts
, specifically with OwidGdocLinkType.Grapher
being passed as an argument to a function. This confirms that the type OwidGdocLinkType
is in use within the same file where string literals were previously found.
Given this information, it seems that the string literals 'grapher' found in the earlier context of db/wpdb.ts
should have been replaced by OwidGdocLinkType.Grapher
. The presence of both the type and the string literals in the same file suggests an inconsistency and indicates that the string literals are likely an oversight.
Based on the evidence from the codebase, it can be concluded that the review comment is correct, and there are indeed issues with the usage of string literals where the type OwidGdocLinkType
should have been used.
Analysis chain
The export of OwidGdocLinkType
has been added. Verify that this new export is being used in place of the previous string literals throughout the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that OwidGdocLinkType is being used in place of string literals
rg --type ts "'gdoc'|'url'|'grapher'|'explorer'" --files-with-matches
Length of output: 228
Script:
#!/bin/bash
# Extract lines from the files where the string literals 'gdoc', 'url', 'grapher', and 'explorer' are used
rg --type ts "'gdoc'|'url'|'grapher'|'explorer'" -C 2
Length of output: 2288
Script:
#!/bin/bash
# Check for usage of OwidGdocLinkType in the codebase
rg "OwidGdocLinkType" --files-with-matches
Length of output: 407
Script:
#!/bin/bash
# Extract specific lines from db/wpdb.ts where OwidGdocLinkType is used
rg "OwidGdocLinkType" db/wpdb.ts -C 2
Length of output: 385
Hey @danyx23, I thought the best way to review this code would be to build with it. I'm almost done migrating the
|
77143f5
to
baa468d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
db/sql-ts/sql-ts-config.json
is excluded by:!**/*.json
package.json
is excluded by:!**/*.json
yarn.lock
is excluded by:!**/*.lock
Files selected for processing (46)
- adminSiteServer/apiRouter.ts (2 hunks)
- baker/algolia/indexChartsToAlgolia.ts (2 hunks)
- db/db.ts (2 hunks)
- db/model/Link.ts (1 hunks)
- db/sql-ts/README.md (1 hunks)
- db/sql-ts/template.handlebars (1 hunks)
- db/tests/basic.test.ts (1 hunks)
- db/wpdb.ts (2 hunks)
- packages/@ourworldindata/components/src/GdocsUtils.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Charts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Entities.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Images.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Origins.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Posts.ts (2 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sources.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Tags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Users.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Variables.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Tag.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Various.ts (1 hunks)
- packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts (3 hunks)
- packages/@ourworldindata/types/src/index.ts (4 hunks)
Files skipped from review as they are similar to previous changes (43)
- adminSiteServer/apiRouter.ts
- baker/algolia/indexChartsToAlgolia.ts
- db/model/Link.ts
- db/sql-ts/README.md
- db/sql-ts/template.handlebars
- db/tests/basic.test.ts
- db/wpdb.ts
- packages/@ourworldindata/components/src/GdocsUtils.ts
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts
- packages/@ourworldindata/types/src/dbTypes/Charts.ts
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts
- packages/@ourworldindata/types/src/dbTypes/Entities.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts
- packages/@ourworldindata/types/src/dbTypes/Images.ts
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts
- packages/@ourworldindata/types/src/dbTypes/Origins.ts
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts
- packages/@ourworldindata/types/src/dbTypes/Posts.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts
- packages/@ourworldindata/types/src/dbTypes/Sources.ts
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Users.ts
- packages/@ourworldindata/types/src/dbTypes/Variables.ts
- packages/@ourworldindata/types/src/domainTypes/Tag.ts
- packages/@ourworldindata/types/src/domainTypes/Various.ts
- packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts
Additional comments: 5
packages/@ourworldindata/types/src/dbTypes/Tags.ts (2)
- 2-2: The introduction of a constant for the table name is a good practice for maintainability and avoiding hard-coded strings.
- 13-13: Using
Required<TagsRowForInsert>
to createTagsRow
ensures that all optional properties inTagsRowForInsert
are required inTagsRow
, which is a clean way to differentiate between creating and retrieving records.packages/@ourworldindata/types/src/index.ts (3)
- 52-52: Replacing the export of
Tag
with the newTag
fromdomainTypes/Tag.js
is a significant change. Ensure that all references to the oldTag
type have been updated accordingly.Verification successful
The verification process did not find any instances of the
Tag
type being imported from an incorrect location. This suggests that all references to theTag
type have been successfully updated to the newTag
fromdomainTypes/Tag.js
.* 279-279: The export of `OwidGdocLinkType` is consistent with the PR's goal of replacing string literals with enums for type safety. Ensure that this enum is used wherever applicable. * 385-607: The addition of exports for various database table types is in line with the PR's objectives to introduce TypeScript types for database tables. This should improve type safety and developer productivity.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all references to the old Tag type have been updated to the new Tag type from domainTypes/Tag.js rg --type ts "Tag" --files-with-matchesLength of output: 2688
Script:
#!/bin/bash # Check the import statements for the Tag type in each of the listed files to ensure they are importing from domainTypes/Tag.js files=( "site/ChartsIndexPage.tsx" "site/viteUtils.tsx" "site/viteUtils.test.ts" "site/search/searchTypes.ts" "site/search/SearchPanel.tsx" "site/gdocs/components/ProminentLink.tsx" "site/gdocs/pages/DataInsight.tsx" "site/blocks/StickyNav.tsx" "site/Head.tsx" "site/DataPageV2.tsx" "site/DataPageV2Content.tsx" "packages/@ourworldindata/types/src/index.ts" "packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts" "packages/@ourworldindata/types/src/gdocTypes/Datapage.ts" "packages/@ourworldindata/types/src/domainTypes/Various.ts" "packages/@ourworldindata/types/src/domainTypes/Tag.ts" "packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts" "packages/@ourworldindata/types/src/dbTypes/Tags.ts" "packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts" "packages/@ourworldindata/types/src/dbTypes/PostTags.ts" "packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts" "packages/@ourworldindata/types/src/dbTypes/ChartTags.ts" "packages/@ourworldindata/types/src/OwidVariable.ts" "packages/@ourworldindata/grapher/src/core/GrapherAnalytics.ts" "packages/@ourworldindata/grapher/src/core/Grapher.tsx" "packages/@ourworldindata/components/src/TextWrap/TextWrap.tsx" "db/wpdb.ts" "db/migration/1658188348638-MarkdownSafety.ts" "db/migration/1701450183524-TagTopicSlug.ts" "db/migration/1698301390787-TopicTagsDisplayOrder.ts" "db/migration/1694348415243-AddIsIsApprovedToChartTags.ts" "db/migration/1692801236245-AddIsTopicToTag.ts" "db/migration/1680210665372-PostsGdocsXTags.ts" "db/migration/1675374345914-ChartTagIsKeyChartNonNull.ts" "db/model/Post.ts" "db/model/GdocXTag.ts" "db/model/Dataset.ts" "db/model/Chart.ts" "db/migrateWpPostsToArchieMl.ts" "db/model/Gdoc/GdocFactory.ts" "db/model/Gdoc/htmlToEnriched.test.ts" "db/model/Gdoc/htmlToEnriched.ts" "db/model/Gdoc/GdocPost.ts" "db/model/Gdoc/GdocBase.ts" "db/analyzeWpPosts.ts" "baker/GrapherBakingUtils.ts" "baker/formatting.test.ts" "baker/batchTagWithGpt.ts" "baker/algolia/indexChartsToAlgolia.ts" "baker/algolia/indexToAlgolia.tsx" "baker/algolia/configureAlgolia.ts" "baker/GrapherBaker.tsx" "datapage/Datapage.ts" "baker/siteRenderers.tsx" "adminSiteClient/TagsIndexPage.tsx" "adminSiteClient/TagEditPage.tsx" "adminSiteClient/TagBucketSortingIcon.tsx" "adminSiteClient/TagBadge.tsx" "adminSiteClient/PostsIndexPage.tsx" "adminSiteClient/GdocsStore.tsx" "adminSiteClient/GdocsPreviewPage.tsx" "adminSiteClient/GdocsIndexPage.tsx" "adminSiteServer/apiRouter.ts" "adminSiteClient/EditableTags.tsx" "adminSiteClient/EditTags.tsx" "adminSiteClient/DatasetList.tsx" "adminSiteClient/DatasetEditPage.tsx" "adminSiteClient/ChartRow.tsx" "adminSiteServer/authentication.tsx" "adminSiteServer/app.tsx" "adminSiteClient/AdminSidebar.tsx" "adminSiteServer/adminRouter.tsx" "adminSiteClient/AdminApp.tsx" "adminSiteClient/ChartList.tsx" ) for file in "${files[@]}"; do if rg --type ts "from '.*/Tag'" "$file"; then echo "$file imports Tag from an incorrect location." fi doneLength of output: 10265
export interface TagsRowForInsert { | ||
createdAt?: Date | ||
id?: number | ||
isBulkImport?: number | ||
name: string | ||
createdAt: Date | ||
updatedAt: Date | ||
parentId: number | ||
isBulkImport: boolean | ||
specialType: string | ||
slug: string | null | ||
parentId?: number | null | ||
slug?: string | null | ||
specialType?: string | null | ||
updatedAt?: Date | null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The database schema for the tags
table indicates that the createdAt
and updatedAt
fields are not nullable. Therefore, unless there is ORM behavior that automatically populates these fields, they should not be optional in the TagsRowForInsert
interface. The id
field is typically auto-incremented and may be optional in the interface if the ORM or database layer handles it automatically. The isBulkImport
, parentId
, and specialType
fields are correctly marked as optional. The slug
field's optionality cannot be verified without further context as it is not mentioned in the schema.
Based on this information, the review comment is partially correct. The createdAt
and updatedAt
fields should be reviewed to ensure they align with the intended ORM behavior and use cases.
Analysis chain
The TagsRowForInsert
interface makes several fields optional which were not optional in the previous Tag
interface. This change should be verified to ensure it aligns with the actual database schema and intended use cases.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the fields marked as optional in TagsRowForInsert match the database schema
ast-grep --lang typescript --pattern $'interface TagsRowForInsert {
createdAt?: $_
id?: $_
isBulkImport?: $_
$$$
}'
Length of output: 909
Script:
#!/bin/bash
# Search for database schema or migration files that define the structure of the `tags` table
rg "CREATE TABLE.*tags" --files-with-matches
Length of output: 188
Script:
#!/bin/bash
# Alternatively, search for any TypeScript type or interface definitions that might correspond to the database schema for tags
ast-grep --lang typescript --pattern $'interface $_ {
$$$
tags($_) {
$$$
}
$$$
}'
Length of output: 570
Script:
#!/bin/bash
# Examine the contents of the identified migration files to determine the schema of the `tags` table
cat db/migration/1680210665372-PostsGdocsXTags.ts
cat db/migration/pre-migrations-schema.sql
cat db/migration/1688372371221-AddPresentationToVariables.ts
Length of output: 26850
51be3d9
to
92618af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
db/sql-ts/sql-ts-config.json
is excluded by:!**/*.json
package.json
is excluded by:!**/*.json
yarn.lock
is excluded by:!**/*.lock
Files selected for processing (45)
- adminSiteServer/apiRouter.ts (2 hunks)
- baker/algolia/indexChartsToAlgolia.ts (2 hunks)
- db/model/Link.ts (1 hunks)
- db/sql-ts/README.md (1 hunks)
- db/sql-ts/template.handlebars (1 hunks)
- db/tests/basic.test.ts (1 hunks)
- db/wpdb.ts (2 hunks)
- packages/@ourworldindata/components/src/GdocsUtils.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Charts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Entities.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Images.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Origins.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Posts.ts (2 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sources.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Tags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Users.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Variables.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Tag.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Various.ts (1 hunks)
- packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts (3 hunks)
- packages/@ourworldindata/types/src/index.ts (4 hunks)
Files skipped from review as they are similar to previous changes (44)
- adminSiteServer/apiRouter.ts
- baker/algolia/indexChartsToAlgolia.ts
- db/model/Link.ts
- db/sql-ts/README.md
- db/sql-ts/template.handlebars
- db/tests/basic.test.ts
- db/wpdb.ts
- packages/@ourworldindata/components/src/GdocsUtils.ts
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts
- packages/@ourworldindata/types/src/dbTypes/Charts.ts
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts
- packages/@ourworldindata/types/src/dbTypes/Entities.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts
- packages/@ourworldindata/types/src/dbTypes/Images.ts
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts
- packages/@ourworldindata/types/src/dbTypes/Origins.ts
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts
- packages/@ourworldindata/types/src/dbTypes/Posts.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts
- packages/@ourworldindata/types/src/dbTypes/Sources.ts
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Users.ts
- packages/@ourworldindata/types/src/dbTypes/Variables.ts
- packages/@ourworldindata/types/src/domainTypes/Tag.ts
- packages/@ourworldindata/types/src/domainTypes/Various.ts
- packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts
- packages/@ourworldindata/types/src/index.ts
Additional comments: 3
packages/@ourworldindata/types/src/dbTypes/Tags.ts (3)
- 2-2: The introduction of
TagsRowTableName
as a constant is a good practice for maintaining consistency in table name references throughout the codebase.- 3-11: The
TagsRowForInsert
interface markscreatedAt
,updatedAt
, andid
as optional. Ensure these fields' optionality aligns with the database schema and ORM behavior, especially sincecreatedAt
andupdatedAt
are typically required by the ORM to automatically manage timestamps.Verification successful
The review comment focused on verifying the optionality of
createdAt
,updatedAt
, andid
fields in theTagsRowForInsert
interface against the database schema for the tags table. The provided schema from thepre-migrations-schema.sql
file shows thetags
table with the following relevant definitions:
id
int NOT NULL AUTO_INCREMENT,createdAt
datetime NOT NULL,updatedAt
datetime NOT NULL,This schema confirms that
id
,createdAt
, andupdatedAt
are indeed not optional in the database schema for the tags table—they are required and automatically managed (withid
being auto-incremented andcreatedAt
,updatedAt
having NOT NULL constraints).Given this information, the
TagsRowForInsert
interface correctly markscreatedAt
andupdatedAt
as optional for insert operations, likely because these values are automatically generated by the database or ORM during the insert process. Theid
field is also optional in the interface, aligning with the fact that it is auto-incremented by the database and not required to be explicitly provided during insert operations.Therefore, the interface's design regarding the optionality of these fields aligns with the database schema and typical ORM behavior, where
createdAt
andupdatedAt
are managed automatically, andid
is auto-generated.* 13-13: Using `export type TagsRow = Required` to create a non-optional version of the `TagsRowForInsert` interface is an effective way to differentiate between creating new rows and fully populated row objects. This approach enhances type safety and clarity in code that interacts with the database.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the optionality of createdAt, updatedAt, and id fields in the database schema for the tags table rg "CREATE TABLE.*tags" --files-with-matchesLength of output: 188
Script:
#!/bin/bash # Inspect the most relevant migration file for the tags table schema, focusing on createdAt, updatedAt, and id fields. cat db/migration/pre-migrations-schema.sqlLength of output: 21925
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
db/sql-ts/sql-ts-config.json
is excluded by:!**/*.json
package.json
is excluded by:!**/*.json
yarn.lock
is excluded by:!**/*.lock
Files selected for processing (45)
- adminSiteServer/apiRouter.ts (2 hunks)
- baker/algolia/indexChartsToAlgolia.ts (2 hunks)
- db/model/Link.ts (1 hunks)
- db/sql-ts/README.md (1 hunks)
- db/sql-ts/template.handlebars (1 hunks)
- db/tests/basic.test.ts (1 hunks)
- db/wpdb.ts (2 hunks)
- packages/@ourworldindata/components/src/GdocsUtils.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Charts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Entities.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Images.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Origins.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Posts.ts (2 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sources.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Tags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Users.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Variables.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Tag.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Various.ts (1 hunks)
- packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts (3 hunks)
- packages/@ourworldindata/types/src/index.ts (4 hunks)
Files skipped from review as they are similar to previous changes (44)
- adminSiteServer/apiRouter.ts
- baker/algolia/indexChartsToAlgolia.ts
- db/model/Link.ts
- db/sql-ts/README.md
- db/sql-ts/template.handlebars
- db/tests/basic.test.ts
- db/wpdb.ts
- packages/@ourworldindata/components/src/GdocsUtils.ts
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts
- packages/@ourworldindata/types/src/dbTypes/Charts.ts
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts
- packages/@ourworldindata/types/src/dbTypes/Entities.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts
- packages/@ourworldindata/types/src/dbTypes/Images.ts
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts
- packages/@ourworldindata/types/src/dbTypes/Origins.ts
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts
- packages/@ourworldindata/types/src/dbTypes/Posts.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts
- packages/@ourworldindata/types/src/dbTypes/Sources.ts
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Users.ts
- packages/@ourworldindata/types/src/dbTypes/Variables.ts
- packages/@ourworldindata/types/src/domainTypes/Tag.ts
- packages/@ourworldindata/types/src/domainTypes/Various.ts
- packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts
- packages/@ourworldindata/types/src/index.ts
Additional comments: 3
packages/@ourworldindata/types/src/dbTypes/Tags.ts (3)
- 2-2: The introduction of
TagsRowTableName
as a constant is a good practice for maintaining consistency and avoiding hard-coded strings throughout the codebase. This change enhances maintainability and reduces the risk of typos in table name references.- 3-11: The
TagsRowForInsert
interface markscreatedAt
,updatedAt
, andid
as optional. This design choice should align with the ORM's behavior and the database schema. Specifically, ifcreatedAt
andupdatedAt
are automatically managed by the ORM, marking them as optional is appropriate. However, ensure that this aligns with the actual database schema, where these fields are typically not nullable. Theid
field being optional is standard for insert operations, assuming it's auto-generated by the database.Verification successful
The database schema for the
tags
table confirms that thecreatedAt
andupdatedAt
fields are indeed not nullable, aligning with the optional designation in theTagsRowForInsert
interface, assuming these fields are automatically managed by the ORM. Theid
field is also set to auto-increment, supporting its optional status for insert operations. This matches the expectations set in the review comment, confirming the alignment between the interface design and the database schema.* 13-13: The `TagsRow` type, defined as `Required`, effectively ensures that all fields must be present for a complete row representation. This approach is sound for use cases where a full row object is needed, ensuring type safety and data integrity.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the `createdAt`, `updatedAt`, and `id` fields' optionality against the database schema. rg "CREATE TABLE.*tags" --files-with-matches # Additionally, check ORM documentation or configuration to confirm automatic handling of `createdAt` and `updatedAt`.Length of output: 188
Script:
#!/bin/bash # Inspect the schema definition for the tags table to verify the fields' optionality. cat db/migration/pre-migrations-schema.sql cat db/migration/1688372371221-AddPresentationToVariables.ts cat db/migration/1680210665372-PostsGdocsXTags.tsLength of output: 26850
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
package.json
is excluded by:!**/*.json
yarn.lock
is excluded by:!**/*.lock
Files selected for processing (53)
- adminSiteClient/ChartList.tsx (3 hunks)
- adminSiteClient/ChartRow.tsx (2 hunks)
- adminSiteClient/DatasetEditPage.tsx (3 hunks)
- adminSiteClient/DatasetList.tsx (5 hunks)
- adminSiteClient/EditTags.tsx (3 hunks)
- adminSiteClient/EditableTags.tsx (5 hunks)
- adminSiteClient/PostsIndexPage.tsx (4 hunks)
- adminSiteClient/TagBadge.tsx (2 hunks)
- adminSiteClient/TagEditPage.tsx (4 hunks)
- adminSiteClient/TagsIndexPage.tsx (2 hunks)
- adminSiteServer/apiRouter.ts (5 hunks)
- baker/postUpdatedHook.ts (2 hunks)
- baker/siteRenderers.tsx (2 hunks)
- db/model/Chart.ts (2 hunks)
- db/model/Post.ts (2 hunks)
- db/syncPostsToGrapher.ts (3 hunks)
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Charts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Entities.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Images.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Origins.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Posts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sources.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Tags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Users.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Variables.ts (1 hunks)
- packages/@ourworldindata/types/src/index.ts (4 hunks)
- site/EntriesByYearPage.tsx (1 hunks)
- site/GrapherPage.jsdom.test.tsx (2 hunks)
- site/GrapherPage.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (33)
- adminSiteServer/apiRouter.ts
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts
- packages/@ourworldindata/types/src/dbTypes/Charts.ts
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts
- packages/@ourworldindata/types/src/dbTypes/Entities.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts
- packages/@ourworldindata/types/src/dbTypes/Images.ts
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts
- packages/@ourworldindata/types/src/dbTypes/Origins.ts
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts
- packages/@ourworldindata/types/src/dbTypes/Posts.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts
- packages/@ourworldindata/types/src/dbTypes/Sources.ts
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Users.ts
- packages/@ourworldindata/types/src/dbTypes/Variables.ts
Additional comments: 59
packages/@ourworldindata/types/src/dbTypes/Tags.ts (2)
- 3-13: The
DbInsertTag
interface has been correctly updated toTagsRowForInsert
with optional properties and a new requiredname
field, aligning with the new structure for insert operations. However, ensure all references to this interface are updated accordingly across the codebase.- 13-13: The
DbPlainTag
type being a required version ofDbInsertTag
is correctly updated toTagsRow
, ensuring that all properties are required after insertion. This change enhances type safety and clarity in usage.adminSiteClient/EditTags.tsx (3)
- 4-4: The import statement correctly replaces
ChartTagJoin
withDbChartTagJoin
, aligning with the updated type definitions and ensuring consistency in type usage across the application.- 13-16: The props
tags
,suggestions
, and the parameter type foronAdd
have been correctly updated to useDbChartTagJoin
instead ofChartTagJoin
, ensuring type consistency and correctness in the context of database interactions.- 69-69: The
convertTagToAutocomplete
function's parameter type has been correctly updated toDbChartTagJoin
, ensuring that the function operates on the correct type of tag data.db/model/Post.ts (4)
- 3-3: The import statement correctly updates the type imports to
DbEnrichedPost
andDbRawPost
, reflecting the new naming convention for post types.- 7-10: The
select
function's return type has been correctly updated to useDbRawPost
, ensuring type safety and clarity in the function's purpose and return value.- 65-65: The
getPostRawBySlug
function now correctly returns aDbRawPost
type, aligning with the updated type definitions and enhancing code readability and maintainability.- 70-70: The
getPostEnrichedBySlug
function's return type has been updated toDbEnrichedPost
, ensuring consistency with the new type definitions and improving the clarity of the function's return value.adminSiteClient/TagBadge.tsx (2)
- 3-3: The import statement has been correctly updated to use
DbChartTagJoin
instead ofChartTagJoin
, ensuring that the component uses the correct type for tag data.- 14-14: The type of the
tag
prop in theTagBadge
component has been correctly updated toDbChartTagJoin
, aligning with the updated type definitions and ensuring type consistency across the application.site/GrapherPage.jsdom.test.tsx (2)
- 7-7: The import statement correctly updates the type import to
DbEnrichedPost
, reflecting the new naming convention for enriched post types.- 33-33: The
post
variable has been correctly typed asDbEnrichedPost
, ensuring that the test setup aligns with the updated type definitions and enhances the clarity of the test's intent.adminSiteClient/ChartRow.tsx (3)
- 14-14: The type
DbChartTagJoin
is correctly used in theavailableTags
prop, aligning with the updated type definitions and ensuring consistency in type usage across the component.- 26-26: The
saveTags
function has been correctly updated to accept an array ofDbChartTagJoin
instead ofChartTagJoin
, ensuring that the function operates on the correct type of tag data.- 38-38: The
onSaveTags
function's parameter type has been correctly updated toDbChartTagJoin[]
, aligning with the updated type definitions and enhancing the function's clarity and type safety.adminSiteClient/DatasetList.tsx (6)
- 8-8: The import statement correctly updates the type import to
DbChartTagJoin
, reflecting the new naming convention for tag types.- 23-23: The
tags
property within theDatasetListItem
interface has been correctly updated to useDbChartTagJoin[]
, ensuring consistency with the updated type definitions.- 33-33: The
availableTags
prop in theDatasetRow
component has been correctly updated to useDbChartTagJoin[]
, aligning with the updated type definitions and ensuring type consistency across the application.- 39-39: The
saveTags
function within theDatasetRow
component now correctly accepts an array ofDbChartTagJoin
instead ofChartTagJoin
, ensuring that the function operates on the correct type of tag data.- 51-51: The
onSaveTags
callback function's parameter type has been correctly updated toDbChartTagJoin[]
, aligning with the updated type definitions and enhancing the function's clarity and type safety.- 108-108: The
availableTags
observable within theDatasetList
component has been correctly typed asDbChartTagJoin[]
, ensuring consistency with the updated type definitions and enhancing the clarity of the component's state management.adminSiteClient/ChartList.tsx (3)
- 7-7: The import statement correctly updates the type import to
DbChartTagJoin
, reflecting the new naming convention for tag types and ensuring consistency in type usage across the component.- 31-31: The
tags
property within theChartListItem
interface has been correctly updated to useDbChartTagJoin[]
, aligning with the updated type definitions and ensuring type consistency across the application.- 43-43: The
availableTags
observable within theChartList
component has been correctly typed asDbChartTagJoin[]
, ensuring consistency with the updated type definitions and enhancing the clarity of the component's state management.site/EntriesByYearPage.tsx (2)
- 2-2: The import statement correctly updates the type import to
DbEnrichedPost
, reflecting the new naming convention for enriched post types and ensuring consistency in type usage across the component.- 8-8: The
Entry
type has been correctly updated to useDbEnrichedPost
, ensuring that the component operates on the correct type of post data and aligns with the updated type definitions.adminSiteClient/TagsIndexPage.tsx (2)
- 8-8: The import statement correctly updates the type import to
DbChartTagJoin
, reflecting the new naming convention for tag types and ensuring consistency in type usage across the component.- 149-149: The type of the
tag
property in theTagBadge
component has been correctly updated toDbChartTagJoin
when passing theparent
tag, aligning with the updated type definitions and ensuring type consistency across the application.site/GrapherPage.tsx (2)
- 11-11: The import statement correctly updates the type import to
DbEnrichedPost
, reflecting the new naming convention for enriched post types and ensuring consistency in type usage across the component.- 37-37: The type of the
post
property in theGrapherPage
component's props has been correctly updated toDbEnrichedPost
, aligning with the updated type definitions and ensuring type consistency across the application.adminSiteClient/EditableTags.tsx (6)
- 8-8: The import statement correctly updates the type import to
DbChartTagJoin
, reflecting the new naming convention for tag types and ensuring consistency in type usage across the component.- 23-25: The props
tags
,suggestions
, and theonSave
callback parameter have been correctly updated to useDbChartTagJoin[]
, ensuring type consistency and correctness in the context of tag operations.- 37-37: The
tags
observable has been correctly typed asDbChartTagJoin[]
, aligning with the updated type definitions and enhancing the clarity of the component's state management.- 39-39: The
onAddTag
function's parameter type has been correctly updated toDbChartTagJoin
, ensuring that the function operates on the correct type of tag data.- 95-95: The
json
variable within theonSuggest
function has been correctly typed asRecord<"topics", DbChartTagJoin[]>
, ensuring that the function processes the correct type of tag data.- 193-207: Utility functions for filtering and setting default values on tags have been correctly updated to operate on
DbChartTagJoin
instead ofChartTagJoin
, ensuring that these functions align with the updated type definitions and enhance the component's functionality.baker/postUpdatedHook.ts (2)
- 8-8: The import statement correctly updates the type import to
DbEnrichedPost
, reflecting the new naming convention for enriched post types and ensuring consistency in type usage across the file.- 132-132: The casting of
postRow
toDbEnrichedPost
aligns with the updated type definitions, ensuring that the variable is treated with the correct type throughout its usage in the file.adminSiteClient/TagEditPage.tsx (4)
- 5-5: The import statement correctly updates
ChartTagJoin
toDbChartTagJoin
, aligning with the PR's objective to standardize database table types.- 21-22: The
TagPageData
interface correctly updates the types ofchildren
andpossibleParents
fromChartTagJoin[]
toDbChartTagJoin[]
, reflecting the new standardized types.- 177-177: The type casting of
parentTag
within theTagEditor
component correctly usesDbChartTagJoin
, ensuring consistency with the updated type definitions.- 210-210: The type casting of
tag
within theTagEditor
component's map function correctly usesDbChartTagJoin
, aligning with the updated type definitions.db/model/Chart.ts (2)
- 20-20: The import statement correctly updates
ChartTagJoin
toDbChartTagJoin
, aligning with the PR's objective to standardize database table types.- 119-122: The
setTags
method signature within theChart
class correctly updates the type of thetags
parameter fromChartTagJoin
toDbChartTagJoin
, ensuring type consistency.adminSiteClient/PostsIndexPage.tsx (3)
- 6-6: The import statement correctly updates
ChartTagJoin
toDbChartTagJoin
, aligning with the PR's objective to standardize database table types.- 76-76: The
saveTags
method in thePostRow
class correctly updates its parameter type toDbChartTagJoin[]
, reflecting the new standardized types.- 88-88: The
onSaveTags
method in thePostRow
class correctly updates its parameter type toDbChartTagJoin[]
, ensuring consistency with the updated type definitions.db/syncPostsToGrapher.ts (3)
- 11-11: The import statement correctly updates
PostRowEnriched
toDbEnrichedPost
, aligning with the PR's objective to standardize database table types.- 121-121: The
getLinksToAddAndRemoveForPost
function correctly updates its parameter type toDbEnrichedPost
, ensuring consistency with the updated type definitions.- 317-317: The mapping to
DbEnrichedPost[]
for thetoInsert
variable correctly reflects the updated type definitions, ensuring type consistency.adminSiteClient/DatasetEditPage.tsx (3)
- 8-8: The import statement correctly updates
ChartTagJoin
toDbChartTagJoin
, aligning with the PR's objective to standardize database table types.- 69-69: The
DatasetEditable
class correctly updates the type of thetags
property fromChartTagJoin[]
toDbChartTagJoin[]
, reflecting the new standardized types.- 86-86: The
onSaveTags
method in theDatasetTagEditor
class correctly updates its parameter type toDbChartTagJoin[]
, ensuring consistency with the updated type definitions.packages/@ourworldindata/types/src/index.ts (3)
- 52-52: The export of
Tag
has been moved from"dbTypes/Tags.js"
to"domainTypes/Tag.js"
. Ensure that all references toTag
throughout the codebase have been updated to reflect this change.- 279-279: The export of
OwidGdocLinkType
has been added. Verify that this new export is being used in place of the previous string literals throughout the codebase.- 385-602: The addition of numerous exports related to database types (e.g.,
DbPlainAnalyticsPageview
,DbInsertChartDimension
, etc.) significantly enhances the type safety for database interactions. Ensure that these new types are being utilized in all relevant database interaction code paths to replace any loosely typed or any previously existing types that might have been less descriptive.baker/siteRenderers.tsx (2)
- 58-58: The replacement of
PostRowRaw
withDbRawPost
in the import statement aligns with the PR's objective to standardize database interaction types. Ensure that all instances wherePostRowRaw
was used are now correctly usingDbRawPost
.Verification successful
The search for
PostRowRaw
in TypeScript files did not yield any results, indicating thatPostRowRaw
is no longer used in the TypeScript files of the project. This supports the initial review comment thatDbRawPost
has replacedPostRowRaw
as part of the effort to standardize database interaction types across the application.* 461-461: The type annotation for `entries` in the `entriesByYearPage` function has been updated to use `DbRawPost` instead of `PostRowRaw`. This change is consistent with the PR's goal of enhancing type safety. Confirm that the function's logic correctly handles the structure of `DbRawPost`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that DbRawPost is now being used instead of PostRowRaw rg 'PostRowRaw' --type tsLength of output: 25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
db/sql-ts/sql-ts-config.json
is excluded by:!**/*.json
package.json
is excluded by:!**/*.json
yarn.lock
is excluded by:!**/*.lock
Files selected for processing (63)
- adminSiteClient/ChartList.tsx (3 hunks)
- adminSiteClient/ChartRow.tsx (2 hunks)
- adminSiteClient/DatasetEditPage.tsx (3 hunks)
- adminSiteClient/DatasetList.tsx (5 hunks)
- adminSiteClient/EditTags.tsx (3 hunks)
- adminSiteClient/EditableTags.tsx (5 hunks)
- adminSiteClient/PostsIndexPage.tsx (4 hunks)
- adminSiteClient/TagBadge.tsx (2 hunks)
- adminSiteClient/TagEditPage.tsx (4 hunks)
- adminSiteClient/TagsIndexPage.tsx (2 hunks)
- adminSiteServer/apiRouter.ts (7 hunks)
- baker/algolia/indexChartsToAlgolia.ts (2 hunks)
- baker/postUpdatedHook.ts (2 hunks)
- baker/siteRenderers.tsx (2 hunks)
- db/model/Chart.ts (2 hunks)
- db/model/Link.ts (1 hunks)
- db/model/Post.ts (2 hunks)
- db/sql-ts/README.md (1 hunks)
- db/sql-ts/template.handlebars (1 hunks)
- db/syncPostsToGrapher.ts (3 hunks)
- db/tests/basic.test.ts (1 hunks)
- db/wpdb.ts (2 hunks)
- packages/@ourworldindata/components/src/GdocsUtils.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Charts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Entities.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Images.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Origins.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Posts.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Sources.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Tags.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Users.ts (1 hunks)
- packages/@ourworldindata/types/src/dbTypes/Variables.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Tag.ts (1 hunks)
- packages/@ourworldindata/types/src/domainTypes/Various.ts (1 hunks)
- packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts (3 hunks)
- packages/@ourworldindata/types/src/index.ts (4 hunks)
- site/EntriesByYearPage.tsx (1 hunks)
- site/GrapherPage.jsdom.test.tsx (2 hunks)
- site/GrapherPage.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (60)
- adminSiteClient/ChartList.tsx
- adminSiteClient/ChartRow.tsx
- adminSiteClient/DatasetList.tsx
- adminSiteClient/EditTags.tsx
- adminSiteClient/EditableTags.tsx
- adminSiteClient/PostsIndexPage.tsx
- adminSiteClient/TagBadge.tsx
- adminSiteClient/TagEditPage.tsx
- adminSiteClient/TagsIndexPage.tsx
- adminSiteServer/apiRouter.ts
- baker/algolia/indexChartsToAlgolia.ts
- baker/postUpdatedHook.ts
- baker/siteRenderers.tsx
- db/model/Chart.ts
- db/model/Link.ts
- db/model/Post.ts
- db/sql-ts/README.md
- db/sql-ts/template.handlebars
- db/syncPostsToGrapher.ts
- db/tests/basic.test.ts
- db/wpdb.ts
- packages/@ourworldindata/components/src/GdocsUtils.ts
- packages/@ourworldindata/types/src/dbTypes/AnalyticsPageviews.ts
- packages/@ourworldindata/types/src/dbTypes/ChartDimensions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/ChartSlugRedirects.ts
- packages/@ourworldindata/types/src/dbTypes/ChartTags.ts
- packages/@ourworldindata/types/src/dbTypes/Charts.ts
- packages/@ourworldindata/types/src/dbTypes/CountryLatestData.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetFiles.ts
- packages/@ourworldindata/types/src/dbTypes/DatasetTags.ts
- packages/@ourworldindata/types/src/dbTypes/Datasets.ts
- packages/@ourworldindata/types/src/dbTypes/Entities.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerCharts.ts
- packages/@ourworldindata/types/src/dbTypes/ExplorerVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Explorers.ts
- packages/@ourworldindata/types/src/dbTypes/Images.ts
- packages/@ourworldindata/types/src/dbTypes/Namespaces.ts
- packages/@ourworldindata/types/src/dbTypes/Origins.ts
- packages/@ourworldindata/types/src/dbTypes/OriginsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/PostTags.ts
- packages/@ourworldindata/types/src/dbTypes/Posts.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsVariablesFaqs.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXImages.ts
- packages/@ourworldindata/types/src/dbTypes/PostsGdocsXTags.ts
- packages/@ourworldindata/types/src/dbTypes/PostsLinks.ts
- packages/@ourworldindata/types/src/dbTypes/Sessions.ts
- packages/@ourworldindata/types/src/dbTypes/Sources.ts
- packages/@ourworldindata/types/src/dbTypes/SuggestedChartRevisions.ts
- packages/@ourworldindata/types/src/dbTypes/TagsVariables.ts
- packages/@ourworldindata/types/src/dbTypes/Users.ts
- packages/@ourworldindata/types/src/dbTypes/Variables.ts
- packages/@ourworldindata/types/src/domainTypes/Tag.ts
- packages/@ourworldindata/types/src/domainTypes/Various.ts
- packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts
- site/EntriesByYearPage.tsx
- site/GrapherPage.jsdom.test.tsx
- site/GrapherPage.tsx
Additional comments: 7
packages/@ourworldindata/types/src/dbTypes/Tags.ts (1)
- 3-13: The introduction of
DbInsertTag
andDbPlainTag
interfaces aligns with the PR's objective of enhancing type safety and developer productivity by generating TypeScript types for database tables. However, ensure that all instances where these types are used are updated accordingly to prevent type mismatches.adminSiteClient/DatasetEditPage.tsx (3)
- 8-8: The import statement correctly replaces
ChartTagJoin
withDbChartTagJoin
, aligning with the PR's objectives of enhancing type safety through generated TypeScript types. Ensure all usages ofDbChartTagJoin
are consistent throughout the project.- 69-69: The change in the
tags
property type of theDatasetEditable
class toDbChartTagJoin[]
is consistent with the PR's goal of improving type safety. Verify that this change does not introduce any issues where thetags
property is accessed or modified.- 86-86: The modification of the
onSaveTags
method parameter type toDbChartTagJoin[]
in theDatasetTagEditor
class is in line with the PR's objectives. This change enhances type safety and developer experience by ensuring consistency with the updated types. Confirm that theonSaveTags
method's implementation correctly handles the updated type.packages/@ourworldindata/types/src/index.ts (3)
- 52-52: The replacement of the
Tag
export from"dbTypes/Tags.js"
with"domainTypes/Tag.js"
is a significant change that aligns with the PR's goal of refining type annotations and method signatures. Ensure that this change is reflected across all files that importTag
.- 279-279: The addition of
OwidGdocLinkType
from"gdocTypes/Gdoc.js"
is a notable enhancement that likely contributes to the PR's objectives of improving type safety and developer productivity. Ensure that the new type is utilized wherever applicable to replace any hardcoded string literals that represent the same concepts.- 385-605: The addition of a significant number of new exports related to database types, including rows for various entities, is a comprehensive update that enhances the project's type safety and data handling capabilities. This change aligns with the PR's objectives and should be carefully integrated to ensure compatibility and consistency across the project.
@@ -1,11 +1,13 @@ | |||
/** the entity in the `tags` table */ | |||
export interface Tag { | |||
id: number | |||
export const TagsTableName = "tags" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constant TagsTableName
should be renamed to TagsRowTableName
to match the updated naming convention and ensure consistency across the codebase.
- export const TagsTableName = "tags"
+ export const TagsRowTableName = "tags"
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
export const TagsTableName = "tags" | |
export const TagsRowTableName = "tags" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- baker/SiteBaker.tsx (2 hunks)
- baker/algolia/indexToAlgolia.tsx (1 hunks)
- baker/sitemap.ts (1 hunks)
- db/db.ts (1 hunks)
- db/refreshPageviewsFromDatasette.ts (1 hunks)
Additional comments: 6
baker/sitemap.ts (1)
- 61-72: Passing the
knex
instance as a parameter todb.getSlugsWithPublishedGdocsSuccessors
and usingknex.raw
instead ofdb.knexRaw
are good practices for decoupling database interactions. This makes the code more modular and easier to test. Ensure that all other database interactions in the project follow this pattern for consistency.db/db.ts (2)
- 111-125: The modifications to
knexRaw
and the addition ofknexRawFirst
to accept aknex
parameter are positive changes that increase the functions' reusability and testability by allowing them to work with anyknex
instance. This is a good practice for database interaction functions.- 133-141: Modifying
getSlugsWithPublishedGdocsSuccessors
to accept aknex
parameter is a good practice, aligning with the principle of dependency injection. This change enhances the function's modularity and makes it easier to test by allowing it to operate with anyknex
instance.baker/algolia/indexToAlgolia.tsx (1)
- 193-195: Passing the
knexInstance
as a parameter togetSlugsWithPublishedGdocsSuccessors
is a good practice that aligns with the principle of dependency injection, enhancing the function's modularity and testability. Ensure that all other database interactions in the project follow this pattern for consistency.baker/SiteBaker.tsx (2)
- 414-416: Passing the
knex
instance as a parameter togetSlugsWithPublishedGdocsSuccessors
is a good practice that enhances the modularity and testability of the code. This change aligns with the principle of dependency injection, allowing the function to operate with anyknex
instance.- 723-732: Directly using the
knex
instance for database queries inbakeGoogleScholar
is a good practice that improves the modularity of the code. Ensure that all database interactions in the project follow this pattern for consistency and ease of testing.
db/refreshPageviewsFromDatasette.ts
Outdated
knex.transaction(async (trx) => { | ||
await db.knexRaw("TRUNCATE TABLE analytics_pageviews", trx) | ||
|
||
await trx.batchInsert("analytics_pageviews", onlyValidRows) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introduction of a transaction for inserting data into analytics_pageviews
is a good practice for ensuring atomicity. However, there's no error handling for the transaction. Consider adding a .catch
block to handle any errors that might occur during the transaction.
knex.transaction(async (trx) => {
await db.knexRaw("TRUNCATE TABLE analytics_pageviews", trx)
await trx.batchInsert("analytics_pageviews", onlyValidRows)
+ }).catch((error) => {
+ console.error("Failed to insert CSV data:", error)
+ })
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
knex.transaction(async (trx) => { | |
await db.knexRaw("TRUNCATE TABLE analytics_pageviews", trx) | |
await trx.batchInsert("analytics_pageviews", onlyValidRows) | |
}) | |
knex.transaction(async (trx) => { | |
await db.knexRaw("TRUNCATE TABLE analytics_pageviews", trx) | |
await trx.batchInsert("analytics_pageviews", onlyValidRows) | |
}).catch((error) => { | |
console.error("Failed to insert CSV data:", error) | |
}) |
await db.knexRaw("TRUNCATE TABLE analytics_pageviews", trx) | ||
|
||
await trx.batchInsert("analytics_pageviews", onlyValidRows) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using console.log
for logging successful operations is acceptable for small scripts or during development. However, for production code, especially in a database context, consider implementing a more robust logging mechanism that can provide more context and is easily searchable or filterable.
Consider using a dedicated logging library like Winston or Bunyan that supports different log levels and outputs to various destinations (console, file, external logging services).
This PR add helper types for all db tables that are still in reasonable use, even if they are not touched by the Grapher codebase ATM.
The naming scheme for the types is as follows:
TableNameSingular is the singular case for the table name, e.g.
Variable
for thevariables
MySQL table orPostGdoc
forposts_gdocs
.The DbPlain* variant is for cases where all columns have a primitive type that does not need to be parsed.
The DbRaw* variant is for tables that have complex types (e.g. json columns) that need to be parsed. The resulting type once they have been (manually) parsed is the DbEnriched* variant of the type. Next to the type definitions in ./packages/@ourworldindata/types/src/dbTypes are also the parsing/serializing functions. These are currently explicit functions for each column even if they are for now all implemented as simple
JSON.parse
calls - but in theory they might want to add validation.Finally, the DbInsert* variant is for inserting rows into the database. All fields that correspond to columns that are either nullable or have default values are optional so that the minimal object that is valid in the DB can be expressed correctly but all optional fields can be specified as well.
This PR also adds setup for the
sql-ts
tool that generates typescript types from MySQL tables. This is useful if a substantial number of new tables are added and you want to get a reasonable starting point for the corresponding TS type.