Skip to content
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

feat: adds glob support for dir and ignorePattern #1274

Merged
merged 10 commits into from
Sep 23, 2024

Conversation

benkroeger
Copy link
Contributor

@benkroeger benkroeger commented Sep 18, 2024

adds support to work with glob patterns for dir and ignorePattern as alternative to the current implementation (which uses dir as relative path to cwd() and generates a regex from ignorePattern.

CLI requires to additionally set the flag --use-glob while the RunnerOptions for API have been extended with a useGlob boolean property.

When using glob, migration files are sorted by their absolute path (via String.localeCompare) while setting numeric: true in the Intl.Collator().
The original sorting has not changed (still attempts to extract Date from the filename (either utc or timestamp) and sorts by the date's numeric value if possible - localeCompare otherwise)

This also fixes the most parts (if not all) of #932 and thus makes #1226 obsolete
Seems to make #911 obsolete as well

@Shinigami92 Shinigami92 added c: feature Request for new feature p: 1-normal Nothing urgent labels Sep 18, 2024
@Shinigami92
Copy link
Collaborator

looks like you were working from your old main branch 🙂
please update the branch and resolve the conflict

Signed-off-by: Benjamin Kroeger <benjamin.kroeger@gmail.com>
Copy link

github-actions bot commented Sep 18, 2024

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 92.02% (🎯 90%)
⬇️ -0.01%
3196 / 3473
🟢 Statements 92.02% (🎯 90%)
⬇️ -0.01%
3196 / 3473
🟢 Functions 95.88% (🎯 90%)
⬆️ +0.04%
256 / 267
🟢 Branches 90.32% (🎯 85%)
⬆️ +0.29%
822 / 910
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Unchanged Files
src/db.ts 84.29% 89.28% 87.5% 84.29% 75-77, 107, 110-119, 121-122, 162-164
src/index.ts 100% 100% 100% 100%
src/migration.ts 72.9% 86.79% 66.66% 72.9% 133-136, 157-159, 161-173, 212-218, 223-227, 230, 232-236, 238-245, 248, 251-256, 258-259, 316-317, 343-345, 353-354, 371-374, 403-404
src/migrationBuilder.ts 95.43% 92.85% 88.88% 95.43% 354-358, 497-499, 501-502, 526-527
src/runner.ts 75.43% 59.25% 90% 75.43% 45, 65-66, 75-76, 125-126, 168-171, 180-184, 197, 201-202, 204-206, 208-214, 233, 235-241, 244, 257, 269, 271-277, 279-282, 285-288, 298-299, 308-310, 319, 321, 323-330
src/sqlMigration.ts 90% 100% 80% 90% 45-46, 48-49
src/types.ts 100% 100% 100% 100%
src/operations/sql.ts 100% 100% 100% 100%
src/operations/casts/createCast.ts 100% 100% 100% 100%
src/operations/casts/dropCast.ts 100% 100% 100% 100%
src/operations/casts/index.ts 100% 100% 100% 100%
src/operations/domains/alterDomain.ts 100% 100% 100% 100%
src/operations/domains/createDomain.ts 100% 100% 100% 100%
src/operations/domains/dropDomain.ts 100% 100% 100% 100%
src/operations/domains/index.ts 100% 100% 100% 100%
src/operations/domains/renameDomain.ts 100% 100% 100% 100%
src/operations/domains/shared.ts 100% 100% 100% 100%
src/operations/extensions/createExtension.ts 100% 100% 100% 100%
src/operations/extensions/dropExtension.ts 100% 100% 100% 100%
src/operations/extensions/index.ts 100% 100% 100% 100%
src/operations/extensions/shared.ts 100% 100% 100% 100%
src/operations/functions/createFunction.ts 95.58% 94.44% 100% 95.58% 71-73
src/operations/functions/dropFunction.ts 100% 100% 100% 100%
src/operations/functions/index.ts 100% 100% 100% 100%
src/operations/functions/renameFunction.ts 100% 100% 100% 100%
src/operations/functions/shared.ts 100% 100% 100% 100%
src/operations/grants/grantOnSchemas.ts 100% 100% 100% 100%
src/operations/grants/grantOnTables.ts 100% 100% 100% 100%
src/operations/grants/grantRoles.ts 100% 100% 100% 100%
src/operations/grants/index.ts 100% 100% 100% 100%
src/operations/grants/revokeOnSchemas.ts 100% 100% 100% 100%
src/operations/grants/revokeOnTables.ts 100% 100% 100% 100%
src/operations/grants/revokeRoles.ts 100% 100% 100% 100%
src/operations/grants/shared.ts 95.65% 85.71% 100% 95.65% 71
src/operations/indexes/createIndex.ts 96.29% 95.23% 100% 96.29% 74-75
src/operations/indexes/dropIndex.ts 100% 100% 100% 100%
src/operations/indexes/index.ts 100% 100% 100% 100%
src/operations/indexes/shared.ts 88% 86.95% 100% 88% 22, 32-35, 47
src/operations/materializedViews/alterMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/createMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/dropMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/index.ts 100% 100% 100% 100%
src/operations/materializedViews/refreshMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/renameMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/renameMaterializedViewColumn.ts 100% 100% 100% 100%
src/operations/materializedViews/shared.ts 100% 87.5% 100% 100%
src/operations/operators/addToOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/createOperator.ts 100% 100% 100% 100%
src/operations/operators/createOperatorClass.ts 100% 83.33% 100% 100%
src/operations/operators/createOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/dropOperator.ts 100% 100% 100% 100%
src/operations/operators/dropOperatorClass.ts 100% 100% 100% 100%
src/operations/operators/dropOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/index.ts 100% 100% 100% 100%
src/operations/operators/removeFromOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/renameOperatorClass.ts 100% 100% 100% 100%
src/operations/operators/renameOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/shared.ts 85.71% 75% 100% 85.71% 24-25, 38
src/operations/policies/alterPolicy.ts 100% 100% 100% 100%
src/operations/policies/createPolicy.ts 100% 100% 100% 100%
src/operations/policies/dropPolicy.ts 100% 100% 100% 100%
src/operations/policies/index.ts 100% 100% 100% 100%
src/operations/policies/renamePolicy.ts 100% 100% 100% 100%
src/operations/policies/shared.ts 100% 100% 100% 100%
src/operations/roles/alterRole.ts 100% 100% 100% 100%
src/operations/roles/createRole.ts 100% 75% 100% 100%
src/operations/roles/dropRole.ts 100% 100% 100% 100%
src/operations/roles/index.ts 100% 100% 100% 100%
src/operations/roles/renameRole.ts 100% 100% 100% 100%
src/operations/roles/shared.ts 98.07% 76.92% 100% 98.07% 78
src/operations/schemas/createSchema.ts 100% 100% 100% 100%
src/operations/schemas/dropSchema.ts 100% 100% 100% 100%
src/operations/schemas/index.ts 100% 100% 100% 100%
src/operations/schemas/renameSchema.ts 100% 100% 100% 100%
src/operations/sequences/alterSequence.ts 94.11% 75% 100% 94.11% 23
src/operations/sequences/createSequence.ts 100% 100% 100% 100%
src/operations/sequences/dropSequence.ts 100% 100% 100% 100%
src/operations/sequences/index.ts 100% 100% 100% 100%
src/operations/sequences/renameSequence.ts 100% 100% 100% 100%
src/operations/sequences/shared.ts 78.57% 68.75% 100% 78.57% 41, 43-44, 49-50, 63-64, 69-70
src/operations/tables/addColumns.ts 100% 80% 100% 100%
src/operations/tables/addConstraint.ts 100% 100% 100% 100%
src/operations/tables/alterColumn.ts 88.57% 76.47% 100% 88.57% 75, 82-85, 87-89
src/operations/tables/alterTable.ts 100% 100% 100% 100%
src/operations/tables/createTable.ts 86.2% 66.66% 100% 86.2% 44-48, 65, 75-76
src/operations/tables/dropColumns.ts 100% 100% 100% 100%
src/operations/tables/dropConstraint.ts 100% 100% 100% 100%
src/operations/tables/dropTable.ts 100% 100% 100% 100%
src/operations/tables/index.ts 100% 100% 100% 100%
src/operations/tables/renameColumn.ts 100% 100% 100% 100%
src/operations/tables/renameConstraint.ts 100% 100% 100% 100%
src/operations/tables/renameTable.ts 100% 100% 100% 100%
src/operations/tables/shared.ts 82.31% 78.46% 80% 82.31% 137-138, 141-142, 195-199, 224, 228-229, 248-249, 274-275, 278-285, 288-289, 426-431, 433-436, 438-448, 450-451
src/operations/triggers/createTrigger.ts 86.41% 68.18% 100% 86.41% 53-54, 66-67, 70-71, 74-77, 113
src/operations/triggers/dropTrigger.ts 100% 100% 100% 100%
src/operations/triggers/index.ts 100% 100% 100% 100%
src/operations/triggers/renameTrigger.ts 100% 100% 100% 100%
src/operations/triggers/shared.ts 100% 100% 100% 100%
src/operations/types/addTypeAttribute.ts 100% 100% 100% 100%
src/operations/types/addTypeValue.ts 100% 100% 100% 100%
src/operations/types/createType.ts 100% 100% 100% 100%
src/operations/types/dropType.ts 100% 100% 100% 100%
src/operations/types/dropTypeAttribute.ts 100% 100% 100% 100%
src/operations/types/index.ts 100% 100% 100% 100%
src/operations/types/renameType.ts 100% 100% 100% 100%
src/operations/types/renameTypeAttribute.ts 100% 100% 100% 100%
src/operations/types/renameTypeValue.ts 100% 100% 100% 100%
src/operations/types/setTypeAttribute.ts 100% 100% 100% 100%
src/operations/views/alterView.ts 100% 100% 100% 100%
src/operations/views/alterViewColumn.ts 100% 100% 100% 100%
src/operations/views/createView.ts 100% 100% 100% 100%
src/operations/views/dropView.ts 100% 100% 100% 100%
src/operations/views/index.ts 100% 100% 100% 100%
src/operations/views/renameView.ts 100% 100% 100% 100%
src/operations/views/shared.ts 100% 66.66% 100% 100%
src/utils/PgLiteral.ts 88.88% 100% 75% 88.88% 37-38
src/utils/StringIdGenerator.ts 100% 100% 100% 100%
src/utils/createSchemalize.ts 100% 100% 100% 100%
src/utils/createTransformer.ts 100% 100% 100% 100%
src/utils/decamelize.ts 100% 100% 100% 100%
src/utils/escapeValue.ts 100% 100% 100% 100%
src/utils/formatLines.ts 100% 100% 100% 100%
src/utils/formatParams.ts 100% 100% 100% 100%
src/utils/getMigrationTableSchema.ts 100% 100% 100% 100%
src/utils/getSchemas.ts 100% 100% 100% 100%
src/utils/identity.ts 100% 100% 100% 100%
src/utils/index.ts 100% 100% 100% 100%
src/utils/intersection.ts 100% 100% 100% 100%
src/utils/makeComment.ts 100% 100% 100% 100%
src/utils/quote.ts 100% 100% 100% 100%
src/utils/toArray.ts 100% 100% 100% 100%
src/utils/types.ts 100% 100% 100% 100%
Generated in workflow #1172

bin/node-pg-migrate.ts Outdated Show resolved Hide resolved
docs/src/api.md Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 marked this pull request as draft September 18, 2024 12:05
@Shinigami92
Copy link
Collaborator

Set to draft for now, because some failing tests
Looks like you are already a trusted contributor, so the tests will run automatically 🙂

I gave a really simple first review, but I need to have a look into details, but assume I wont have time before upcoming Saturday, because I will be on a conference

But it looks really promising and potentially even like a better solution that the other strategy ❤️

Signed-off-by: Benjamin Kroeger <benjamin.kroeger@gmail.com>
Signed-off-by: Benjamin Kroeger <benjamin.kroeger@gmail.com>
because the previous implementation used that only on the migration file's basename (which also caused two files with same name but different extensions to be treated as equal priority)

Signed-off-by: Benjamin Kroeger <benjamin.kroeger@gmail.com>
Signed-off-by: Benjamin Kroeger <benjamin.kroeger@gmail.com>
@benkroeger benkroeger marked this pull request as ready for review September 18, 2024 13:19
src/migration.ts Outdated Show resolved Hide resolved
src/migration.ts Outdated Show resolved Hide resolved
src/migration.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/migration.ts Outdated Show resolved Hide resolved
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Signed-off-by: Benjamin Kroeger <benjamin.kroeger@gmail.com>
Copy link
Collaborator

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I had an in depth look and really like the PR 👍
As you already wrote in the ? comments, we are free to change the name or inner structure of these function. Theoretically users could import them because it is exported in package.json, but I would count this more as a legacy backward compatibility and not official support. So I'm free to break even in a minor version bump.

But please you now need to change the both sections of ! and ? comments so they are solved in this PR, before getting merged.
And if you like, you can also directly use the options destructure pattern in this PR, or I could do this in a separate PR later on.

also updates code documentation

Signed-off-by: Benjamin Kroeger <benjamin.kroeger@gmail.com>
src/migration.ts Outdated Show resolved Hide resolved
src/migration.ts Outdated Show resolved Hide resolved
@theoephraim
Copy link
Contributor

It may not matter so much in this case, but I recently had to implement some globbing behaviour and found fdir (https://www.npmjs.com/package/fdir) to be much more performant. Should be a simple swap.

@Shinigami92
Copy link
Collaborator

It may not matter so much in this case, but I recently had to implement some globbing behaviour and found fdir (npmjs.com/package/fdir) to be much more performant. Should be a simple swap.

image

... Lets stay with glob for now, I plan anyways to use glob from node:fs when we are on Node v22 https://nodejs.org/api/fs.html#fsglobpattern-options-callback

@benkroeger
Copy link
Contributor Author

It may not matter so much in this case, but I recently had to implement some globbing behaviour and found fdir (https://www.npmjs.com/package/fdir) to be much more performant. Should be a simple swap.

if you're going with pure js implementations, there is no faster lib that adheres the de-facto-spec as good as this one.
Also see Comparison to Other JavaScript Glob Implementations.

I don't think that performance is that much of the essence here either.

Signed-off-by: Benjamin Kroeger <benjamin.kroeger@gmail.com>
@Shinigami92 Shinigami92 merged commit 6d4570c into salsita:main Sep 23, 2024
34 checks passed
@Shinigami92
Copy link
Collaborator

Might release next few days, I also currently await feedback of #1262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants