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: dbMigrationPathOpt - add db migration path to MapeoProject and MapeoManager #384

Merged
merged 6 commits into from
Nov 22, 2023

Conversation

tomasciccola
Copy link
Contributor

Should close #341

For now:

  • add optional opt for projectMigrationFolder to mapeoProject with default value
  • add optional opt for projectMigrationFolder and clientMigrationFolder to mapeoManager with default values

Since I put a default value to both params I made both of them optionals, but maybe it not the right approach?
Also the types for both are strings and then turned into a path with new URL

* add optional opt for projectMigrationFolder to mapeoProject with default
  value
* add optional opt for projectMigrationFolder and clientMigrationFolder
  to mapeoManager with default values

Since I put a default value to both params I made both of them
optionals, but maybe it not the right approach?
Also the types for both are strings and then turned into a path with
`new URL`
@tomasciccola tomasciccola self-assigned this Nov 21, 2023
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

I don't think the options should be optional with a default value, as it assumes the location of the migrations directory, which is what we're trying to avoid with this change. Will need to update/fix the tests to account for the suggested changes.

Also suggested changing migration to migrations (plural) to be consistent with the option name from drizzle

src/mapeo-manager.js Outdated Show resolved Hide resolved
src/mapeo-manager.js Outdated Show resolved Hide resolved
src/mapeo-manager.js Outdated Show resolved Hide resolved
src/mapeo-manager.js Outdated Show resolved Hide resolved
src/mapeo-manager.js Outdated Show resolved Hide resolved
src/mapeo-manager.js Outdated Show resolved Hide resolved
src/mapeo-project.js Outdated Show resolved Hide resolved
src/mapeo-project.js Outdated Show resolved Hide resolved
src/mapeo-project.js Outdated Show resolved Hide resolved
@tomasciccola tomasciccola requested a review from achou11 November 22, 2023 12:13
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

nice!

@tomasciccola tomasciccola merged commit b3e777d into main Nov 22, 2023
7 checks passed
@tomasciccola tomasciccola deleted the feat/dbMigrationPathOpt branch November 22, 2023 16:01
gmaclennan added a commit that referenced this pull request Nov 23, 2023
* main:
  chore: fix Brittle type definitions (#383)
  feat: dbMigrationPathOpt - add db migration path to `MapeoProject` and `MapeoManager` (#384)
  [OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.2 (#382)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add path pointing to db migrations folder as opts for MapeoManager and MapeoProject
2 participants