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

feat: allow config to be passed via api #36

Closed
wants to merge 1 commit into from

Conversation

powderham
Copy link

Guys,

This is my proposal for creating an instance of migrateMongo from a constructor, passing config via api. It introduces no regressions (Although it does introduce some code duplication)

It's not ready for merging because it has no tests or an updated readme.

Is this a direction we would like to go in, if so I'd appreciate some help getting it ready for a PR.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fa1ea66 on powderham:master into 998006d on seppevs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fa1ea66 on powderham:master into 998006d on seppevs:master.

@coveralls
Copy link

coveralls commented Nov 28, 2018

Coverage Status

Coverage decreased (-23.9%) to 76.08% when pulling 748121d on powderham:master into a297659 on seppevs:master.

@seppevs
Copy link
Owner

seppevs commented Nov 29, 2018

Thank you for your contribution. I will look into it when I find the time.
However, do you mind to leave out babel? Node 7.x or higher offers us enough ES6 goodness imho.

@powderham
Copy link
Author

powderham commented Nov 30, 2018

Yes, i'm happy to leave it out if we decide we want to go that way.

I started using es6 without linting errors then when I tried to import it, thought the quicker (future proof) solution was to add babel rather than go back. If we want to merge my changes in, then we can look at making the code DRY-er and revert back.

Have a look through and see what you think and then we can talk again.

@joelmukuthu
Copy link

Hi,

I'd also recommend leaving babel out, at least out of this PR. It's easier to review your actual changes and you'll get a faster turn-around time. On a related note, generated files (by babel, minifiers etc) shouldn't be checked into git, especially for a module that's only used in node and not on the browser.

Best,
Joel

@ahvetskovich
Copy link

Hi guys, passing config via api is really necessary feature. Is this PR going to be merged?

@salesh
Copy link

salesh commented Feb 21, 2019

Guys can we get this up?

@salesh
Copy link

salesh commented Feb 26, 2019

Bump again!

Is it rly possible that there is no 1h to just check this great contribution of @powderham ?

@seppevs
Copy link
Owner

seppevs commented Feb 26, 2019

Sorry, I'm very busy at the moment.
The problem is that the PR of @powderham added Babel which I do not want. I will try to implement the fix myself (but I have not much time), otherwise .. feel free to submit a PR (without adding Babel stuff).

@powderham
Copy link
Author

powderham commented Apr 17, 2019

Ok guys I'm looking in to this now

@seppevs I have removed babel and added some tests.
As I refer to above, the code still needs some DRYing out (which should cover off the coverage decline) and the README needs updating.

I want to check in with you and everyone else before polishing a solution that's not yet been accepted. Let me know what you think.

@powderham powderham force-pushed the master branch 2 times, most recently from 45985c0 to 322a1f7 Compare April 18, 2019 10:45
@powderham
Copy link
Author

ping @seppevs @ahvetskovich @joelmukuthu @salesh anyone got time to check this?

@Phoscur
Copy link

Phoscur commented Jan 18, 2020

What's the status on this?
Considering this package better than umzug because of the appliedAt tracking!
Would be great to improve wrapping it to a project specific api.

@powderham
Copy link
Author

What's the status on this?
Considering this package better than umzug because of the appliedAt tracking!
Would be great to improve wrapping it to a project specific api.

I forked this with an API if you want to use it now

@seppevs seppevs force-pushed the master branch 3 times, most recently from 952d33c to 0f39678 Compare July 21, 2020 07:51
@seppevs
Copy link
Owner

seppevs commented Jul 21, 2020

I finally found some time and implemented this feature. It's in migrate-mongo v8.1.1 or later.
See this section in the README for more details.

I'm closing this PR.

@seppevs seppevs closed this Jul 21, 2020
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

7 participants