feat: support index based filename naming strategy#1443
Conversation
Shinigami92
changed the title
… using any integer format
Wrapped getNumericPrefix with a try-catch block for now for backwards compatibility. This should be forced to exit early instead.
|
tipp: try to run |
|
To me it appears that the integration tests are broken. They fail with error message I suggest either modifying the integration test file names or using a regular expression to get the number from the filename (I can modify it to the current pull request since this is a breaking change). |
I fear touching existing migration tests, as that could break existing workflows in the user-base. Creating files via CLI is "just" a UX. (At least I assumed that so far 👀) |
|
Then I suggest that I will use the first digits as the prefix aka revision identifier. There is nonetheless a conflict with hard-coded separator and current test cases. Waiting for your confirmation on this. |
|
And I do use cli for the revisions. It's a habit learned by using Alembic for a long time for migrations, since it uses short hashes for tree-like migration versioning kind of in the spirit of Git commit identifiers. |
So, I re-checked (also to check my sanity/assumption 🤣), but yes node-pg-migrate/bin/node-pg-migrate.ts Line 178 in 56b3044 So, the point is, this parameter is only for creation, but for consuming, I think it is just taken as in order of the folders content. Nowadays even with sub-directories as far as I remember, as there were PRs for that recently. So we cant add breaking stuff right now when adding the feature for index based filename. But you could potentially assume, that if users use that newly strategy, the use it consistently. (hopefully) But we (you 😉) need to find a way, to implement the feature without breaking current behavior. |
How would it sound then that instead of index it would be called increment, which would add one to the last found prefix? It would be backwards compatible with both UTC and epoch as well as supporting starting from 0001. There is already a natural sorting since sort is done against the numeric representation anyway, so padding to meet a length is also completely optional. It should - to support the miscellaneous existing custom naming conventiona - also get any numeric prefix regardless of the separator, so that the current acceptance tests do not fail and users don't need to migrate to anything |
Sounds good to me 🙂 I wont even care if we name it increment or index 🤷 |
So that the existing use cases do not break I changed now the strategy to just increment by one the last found migration prefix and to keep the original prefix length for cohesion. I added also a sane starting point (i.e. start from '0001' if no existing migrations were found).
|
@adrenalin do you also want now directly to work on the |
@Shinigami92, as you pointed out earlier this is another pull request and I think that it would require some discussion first. I will open either a thread or an issue so we can have - with luck - more people participating and at least it will leave a track. I noticed that the code coverage could be improved, so I could in the meanwhile write some unit tests (looking in particular at Then there's the issue that it appears that it is not possible to define explicitly the column for foreign keys with an object. It appears that it is only possible to refer with a string ( How should I proceed with these in terms of communication, discussions or issues? |
IMO issues are very good for more short-time paced point-target discussions, while discussions are more for general feedback and longer discussion-flows. |
2 participants
I added a new prefix option: incremental index (as the already existing test cases indicate it is often more convenient to use short numbers for filenames rather than 13+ numbers)
Prefix "index" cannot be used in conjunction with other types since it is a small integer that cannot be in practice compared to timestamp based prefixes, so there are safeguards set in place to prevent mixed usage