Fix migration bug where using -- as an actual string value gets lost #28
Conversation
src/Database.js
Outdated
| migration.down = down.replace(/^--.*?$/gm, '').trim(); // and trim whitespaces | ||
| migration.up = up.replace(/^-- Up.*?$/gm, '').trim(); // Remove comments | ||
| migration.up = migration.up.replace(/^-+$/gm, '').trim(); | ||
| migration.down = down.replace(/^-- Down.*?$/gm, '').trim(); // and trim whitespaces |
There was a problem hiding this comment.
I think you don't need this replace(): down doesn't contain the comment, since we split on /^--\s+?down/mi
src/Database.js
Outdated
| migration.up = up.replace(/^-- Up.*?$/gm, '').trim(); // Remove comments | ||
| migration.up = migration.up.replace(/^-+$/gm, '').trim(); | ||
| migration.down = down.replace(/^-- Down.*?$/gm, '').trim(); // and trim whitespaces | ||
| migration.down = migration.down.replace(/^-+$/gm, '').trim(); // and trim whitespaces |
There was a problem hiding this comment.
Couldn't we keep comments and just do migration.down = down.trim();? 🤔
This would prevent future issues with dashes in values.
|
thanks @BeLi4L for the review - was really busy at work since then, will change now |
|
@theogravity what do you think about detecting comments by using |
|
I'd rather do that approach instead |
| @@ -167,8 +167,8 @@ class Database { | |||
| reject(new Error(message)); | |||
| } else { | |||
| /* eslint-disable no-param-reassign */ | |||
| migration.up = up.replace(/^--.*?$/gm, '').trim(); // Remove comments | |||
| migration.down = down.replace(/^--.*?$/gm, '').trim(); // and trim whitespaces | |||
| migration.up = up.replace(/^-- .*?$/gm, '').trim();// Remove comments | |||
There was a problem hiding this comment.
Thanks @theogravity 😁
Quick quesiton: do we really need to remove comments? Simply doing migration.up = up.trim() (same as migration.down) would prevent future issues like the one I just had with TLS certificates ;)
There was a problem hiding this comment.
i think @koistya could prob chime in on that one
|
I'm going to merge this in - it's been sitting around so long and the tests are not breaking. |
Fixes #27