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

Allows to filter seeds and migrations by extensions #2168

Merged
merged 2 commits into from Jul 24, 2017

Conversation

doomsower
Copy link
Contributor

This PR addresses issue #1922

I added loadExtensions parameter where you can specify allowed extensions of seeds/migrations to be executed. Default value is ['js', 'ts'] as was suggested in this issue, but now I think maybe it should be set to SUPPORTED_EXTENSIONS so the change is non-breaking?

Couldn't find any tests that deal with ts/coffee/etc. migrations, so I don't know where to start from. I.e. how to to check that ts migrations is executed/not executed based on loadExtensions parameter? But this is small change, I think it's ok as is.

If this PR is acceptable, I will submit PR to docs too.

@elhigu
Copy link
Member

elhigu commented Jul 23, 2017

@doomsower Thanks for the PR, contributions are extremely welcome :) I agree, would be better to have SUPPORTED_EXTENSIONS as default value. Otherwise PR looks clean, but it is missing related PR for the documentation and tests.

@doomsower
Copy link
Contributor Author

@elhigu , I changed default extensions to former SUPPORTED_EXTENSIONS. I also added unit tests for _listAll in Seeder and Migrator, which are the only modified functions. They are the first unit test for Seeder and Migrator and they should be reviewed for sure.

Actually, I think it is better not to introduce this extra loadExtensions parameter, and support only one extension at a time by utilizing existing extension parameter. I think it is bad idea to have migrations in different languages at the same, except maybe for some legacy migrations in project that was converted from js to ts, for example. But for such project this would be a breaking change.

If everything is ok, I'll proceed with docs PR

@elhigu
Copy link
Member

elhigu commented Jul 24, 2017

Beautiful, I haven't seen such clean code here for a while 👍 And I agree about the design issue, that it is weird to support multiple language migrations at the same time, but I don't see any strong reason to start limiting it now.

Please proceed and if you see places where things could be made better and more safe for the user it is not out of the question to break backwards compatibility. We have been breaking it quite a lot lately while trying to make lib to work more sane manner, like throwing errors when user does something that is not defined or potentially dangerous behaviour.

doomsower added a commit to doomsower/documentation that referenced this pull request Jul 24, 2017
Added config option to filter seeds and migrations by extension
@doomsower
Copy link
Contributor Author

Thank you!
Here is PR with docs: knex/documentation#46
Any ETA when I can see this published on NPM?

@elhigu
Copy link
Member

elhigu commented Jul 24, 2017

Thanks! Everything seems to be in order. CI have been broken a while so I haven't been able to get some PRs in which were failing (and I've been very busy lately so I haven't got time to do much of maintenance)... so I cannot say anything about schedule yet. Next version will be 0.14.0 since there are again some breaking changes.

@elhigu elhigu merged commit 1c61ad8 into knex:master Jul 24, 2017
elhigu pushed a commit to knex/documentation that referenced this pull request Nov 6, 2017
Added config option to filter seeds and migrations by extension
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.

None yet

2 participants