node-sqlite
node-sqlite copied to clipboard
Add import readMigrations function in the Database class
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!
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
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.
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?
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.
#180
Let's move to another PR to not remove all. And for clear goal purpose