Add import readMigrations function in the Database class #179

Roma-Kyrnis · 2024-01-04T20:55:52Z

Hi, the main point is to import the readMigrations function but for the minor changes, it was updated to remove the comments mechanism and wrote tests!

Can you have a look?
It's better to update early for me because I'm creating a project based on this module.

But if it needs time to check and update this module it's okay)
I agree to apply your suggesting in the Pull Request so I'll update the code

Thank you!

…e comments from migrations

theogravity

I'm not against the idea but I think we need to make some usability improvements

Thanks for also writing tests too

theogravity · 2024-01-04T21:00:22Z

src/utils/migrate.ts


+            delete migration.filename


Why remove it?

For the types consistency.

Do you want to save all parameters from migration that may be add in the future?

So maybe we will make another type in Migration class for the readMigrations function and in the migrate function we will just require needed arguments but not the exact object as MigrationData.

What do you think?

That sounds appropriate

theogravity · 2024-01-04T21:04:48Z

src/Database.ts

+   * @returns List of objects with name, id of migrations and SQL code for up and down migration
+   * You can run up and down with `db.exec(migrations[0].up)` or `db.exec(migrations[0].down)`. Or with other commands that can run SQL code.
+   */
+  readMigrations (migrationPath?: string): Promise<IMigrate.MigrationData[]> {


Is the intent to just read a list of migrations and then execute them?

It almost feels like this should belong in its own Migrations class and the constructor would accept a Database instance

then you can have a method for just reading the list of migrations / status

and then have another method(s) for executing up / down based on the list of migrations passed through

Yes, I understand that it's better. But it'll take some time. So let's me finish my tasks and I plan to make this changes on the next week

Changes:

add Migration class

add run, show, reverse migration functions

add a commander to run these functions to look on the migrations

add config options where to look for Database instance or config options. With default behaviour. But I plan to add config as a parameter in the command line

WDYT?

The overall idea sounds good. I'd have to see the impl / interface to have additional thoughts

Thanks for doing this. We could also probably move the migration logic to the migration class too.

The Database instance can still keep the migrate method for backwards-compat but would call the new Migration class

Hi, I replied a little later but I have time to make changes)

…ve migration's methods from Database, move tests and update tests

Roma-Kyrnis · 2024-01-20T17:53:56Z

Hi, I just moved all the staff to the Migrations class and have updated for prettier-standard.
Can you have a look at the code?

I'll fix what's needed. And I'll use your code structure.

Do we need to bump the versions of the packages in dev dependencies?
Do you know if we need to add editorconfig? And I somehow cannot format the file with Eslint, only with prettier, but it adds unnecessary commas to the end of the line

theogravity · 2024-01-20T20:17:49Z

Just briefly looking at the changes, there's too many things going on.

You have a ton of file changes due to the reformatting. It's fine to want to do this, but it should be done in its own PR. The point of this PR is to create the migration class and doing this makes code review really difficult to see what is the main thing that has changed
You removed the migrate call from the database class. This creates breaking changes as it's not backwards compatible with people who may use the method in their code
The migration class itself is using arrow functions to define the methods. This is inefficient memory-wise as when someone creates multiple new instances of the migration class, it will create an instance of each method, where if you don't so arrow functions it will be on the class prototype instead.

So here's what we need to do here. Let's close this PR and create another one. Don't bother with linting if you can't get it to work, I can do that for you.

Make a PR that only makes the changes necessary for your Migration class to work.

The methods should not use arrow functions (read more about arrow functions vs prototype inheritance in classes)
The database migration method should remain and do what it did before. This might mean you create a new migration instance in that method itself and then execute migrate.
Include the new tests into the PR
Assuming there are no breaking changes (there should be none), a minor version bump would be appropriate.

In general, keep the PRs small (if possible) and specific to what you are doing, one feature at a time.

Thanks for taking the time to do all that work, but let's make sure we follow and learn best practices here. I'm happy to help you get better at it.

Roma-Kyrnis · 2024-01-20T20:37:23Z

Thank you for your kind explanation 😊

I'll check about arrow functions. And I agree to your comments. Let's do the best practice!

Do I need to leave readMigrations function exported in this PR or move to another one?

Do you know a resource where I can read shortly about PRs best practices?

Features:

Migrations class (functions read, run, down and migrate (both down and run) Migrations)
Commander to control run, show, reverse
bump versions of the devDependecies

I see that we need 3 PRs from points above

Notes:

I had some issues with reading sql files and split up and down code. I've used ts file with exports of down and up. But in this code it's working fine. Maybe some difference in versions?
I plan to add a documentations of this work to Readme. Do I need to do it every PR with new changes?

theogravity · 2024-01-20T21:04:58Z

src/Database.ts

-  /**
-   * Performs a database migration.
-   */
-  async migrate (config?: MigrationParams) {


Keep, but we'll create a new instance of your migration class here, like

const migrate = new Migration(<params >) // do migration

I'll do it in another PR

theogravity · 2024-01-20T21:07:01Z

src/Migrations.ts

+
+export class Migrations<
+  Driver extends Database,
+  Config extends IMigrate.MigrationParams = IMigrate.MigrationParams


This is strange. Why would configuration options be a generic?

Oh, I don't understand what I've done. I've just copied from the Database class.

I've read what Generic means and now I understand that we don't need it here because Config is not changing by the user. It's just parameters that the user can set. For the Database, we need generic because a User can use another Driver instead of our but will extends our Database

theogravity · 2024-01-20T21:09:12Z

src/Migrations.ts

+
+  constructor (db: Driver, config?: Config) {
+    this.db = db
+    /** Sequence in the properties definitions is IMPORTANT */


it's not important. if it is then you're doing something wrong

in JS, object property ordering isn't guaranteed

I think I understand what you mean here - you need the default on the migrationsPath. You don't need the comment for that though.

Yes, about default on the migrationsPath. And also for force and table options. Because

const config = { force: true } const configTest = { table: DEFAULT_MIGRATIONS_DIR_NAME, ...config, force: false, migrationsPath: path.resolve( config?.migrationsPath ?? DEFAULT_MIGRATIONS_DIR_PATH ) }

Force will be false

That's why I wrote sequence is important. I don't know how to say it correctly and easy for others to understand. Maybe you can help write a comment if we need one

theogravity · 2024-01-20T21:09:29Z

src/Migrations.ts

+  Config extends IMigrate.MigrationParams = IMigrate.MigrationParams
+> {
+  db: Driver
+  config: Config


Did you mean for this to be IMigrate.MigrationParams?

Yes, I updated it

theogravity · 2024-01-20T21:09:42Z

src/Migrations.ts

+  db: Driver
+  config: Config
+
+  constructor (db: Driver, config?: Config) {


did you mean IMigrate.MigrationParams?

theogravity · 2024-01-20T21:11:31Z

src/Migrations.ts

+   *  { id: 1, name: 'initial', filename: '001-initial.sql', up: ..., down: ... }
+   *  { id: 2, name: 'feature', filename: '002-feature.sql', up: ..., down: ... }
+   */
+  readMigrations = async (): Promise<MigrationData[]> => {


classes in general do not use arrow functions to define methods. there are specific cases where you would use an arrow function, but the methods in this file isn't one of them

Okay, I understand what you mean)

It's very interesting to go deep in learning JS and other languages. I understand now that I'm just using some part that I know how it works for some purposes but not for everything because I don't know in the depth the under hood processes

theogravity · 2024-01-20T21:13:15Z

src/__tests__/Migrations.test.ts

-    })
+    const migrationsInstanceForce = new Migrations(db, { force: true })
+    await migrationsInstanceForce.migrate()
+    console.log('heer')


stray console log

Forgot about this one)

theogravity · 2024-01-20T21:28:01Z

Thank you for your kind explanation 😊

I'll check about arrow functions. And I agree to your comments. Let's do the best practice!

Do I need to leave readMigrations function exported in this PR or move to another one?

I couldn't really understand what you mean, but I might be able to when I see the new PR where it's easier to read the changes and can give you guidance from then.

Do you know a resource where I can read shortly about PRs best practices?

I think this falls under the topic of general software engineering practices. I did try looking up best practices around PRs and feature branches, but they seem to be focused on documentation and formatting.

I think I learned via just code reviews from other peers over time.

Features:

Migrations class (functions read, run, down and migrate (both down and run) Migrations)

Commander to control run, show, reverse

bump versions of the devDependecies

I see that we need 3 PRs from points above

That sounds almost right, although the version bump would happen during the CI process or in the PR itself depending on the project. I don't know what this commander is, but looking forward to knowing more about it!

Notes:

I had some issues with reading sql files and split up and down code. I've used ts file with exports of down and up. But in this code it's working fine. Maybe some difference in versions?

Assuming you mainly copy/pasted the original merge code into your new class, the existing SQL files should work-as-is without modifications. If it's not working, then your new code may have bugs in it.

When you create a new PR for this feature, I can get a better look / understanding of it.

I plan to add a documentations of this work to Readme. Do I need to do it every PR with new changes?

If you're introducing a new feature and it's something developers would access, then the readme would have to be updated with usage and example information.

Roma-Kyrnis · 2024-01-21T17:02:19Z

#180

Roma-Kyrnis · 2024-01-21T17:03:04Z

Let's move to another PR to not remove all. And for clear goal purpose

feat: add import readMigrations function in the Database class; remov…

b6294e9

…e comments from migrations

theogravity requested changes Jan 4, 2024

View reviewed changes

update prettier-standard to use on Object; add Migrations class, remo…

f8cec0b

…ve migration's methods from Database, move tests and update tests

theogravity reviewed Jan 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add import readMigrations function in the Database class #179

Add import readMigrations function in the Database class #179

Roma-Kyrnis commented Jan 4, 2024

theogravity left a comment

theogravity Jan 4, 2024

Roma-Kyrnis Jan 4, 2024

theogravity Jan 4, 2024

theogravity Jan 4, 2024

Roma-Kyrnis Jan 4, 2024

theogravity Jan 4, 2024

Roma-Kyrnis Jan 19, 2024

Roma-Kyrnis commented Jan 20, 2024

theogravity commented Jan 20, 2024 •

edited

Roma-Kyrnis commented Jan 20, 2024

theogravity Jan 20, 2024

Roma-Kyrnis Jan 21, 2024

theogravity Jan 20, 2024

Roma-Kyrnis Jan 21, 2024

theogravity Jan 20, 2024

theogravity Jan 20, 2024

Roma-Kyrnis Jan 21, 2024

Roma-Kyrnis Jan 21, 2024

theogravity Jan 20, 2024

Roma-Kyrnis Jan 21, 2024

theogravity Jan 20, 2024

theogravity Jan 20, 2024

Roma-Kyrnis Jan 21, 2024

theogravity Jan 20, 2024

Roma-Kyrnis Jan 21, 2024

theogravity commented Jan 20, 2024

Roma-Kyrnis commented Jan 21, 2024

Roma-Kyrnis commented Jan 21, 2024

Add import readMigrations function in the Database class #179

Are you sure you want to change the base?

Add import readMigrations function in the Database class #179

Conversation

Roma-Kyrnis commented Jan 4, 2024

theogravity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roma-Kyrnis commented Jan 20, 2024

theogravity commented Jan 20, 2024 • edited

Roma-Kyrnis commented Jan 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theogravity commented Jan 20, 2024

Roma-Kyrnis commented Jan 21, 2024

Roma-Kyrnis commented Jan 21, 2024

theogravity commented Jan 20, 2024 •

edited