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

[WIP] Reorganize and refactor #20

Closed
wants to merge 22 commits into from
Closed

[WIP] Reorganize and refactor #20

wants to merge 22 commits into from

Conversation

paolochiodi
Copy link
Collaborator

WIP refresh of the store to adhere to new stores structure and make it work with newer versions of seneca and node (builds on top of #19)

@guyellis
Copy link
Contributor

After setting up MySQL I tested this with npm test and out-the-box without any changes the tests don't pass on Node versions 0.10.40, 0.12.7 and 4.0.0

@mcdonnelldean
Copy link
Contributor

@paolochiodi @colmharte Is this ok to be merged?

@paolochiodi
Copy link
Collaborator Author

Mostly. IIRC package.json should point to the now-released version of seneca store test, and README needs some updates

@mcdonnelldean
Copy link
Contributor

Awesome, I'm going push the new doc after lunch for this so. Just wanted to make sure 💃

@mcdonnelldean
Copy link
Contributor

Readme updated as of #21

@mcdonnelldean
Copy link
Contributor

@paolochiodi Can you check the conflicts on this?

@paolochiodi
Copy link
Collaborator Author

@mcdonnelldean I'll give a look to this, but I'm wondering if it's worth the effort to release a version now or if it's better to wait that this senecajs/seneca-store-test#18 gets merged and the new version of seneca-store-test gets published

This makes the store compatible with seneca 0.6

Seneca 0.6 expects the returned object to store methods to be an array,
an entity or null.
There's still no direction about what should be returned in the remove
callback (pg store returns number of deleted rows, mongo the deleted
entity).
Because returning the deleted entity will require two different queries
(thus double roundtrip to the db) we are switching to returning null
until the proper design gets defined
Also remove duplicate tests
@paolochiodi
Copy link
Collaborator Author

rebased to fix conflicts

@ckiss
Copy link
Contributor

ckiss commented Feb 4, 2016

@paolochiodi I'll do some maintenance work on this. can you please solve the conflicts again? thanks :)

@ckiss
Copy link
Contributor

ckiss commented Feb 8, 2016

@paolochiodi any news on this? can you please rebase to fix conflicts?

@paolochiodi
Copy link
Collaborator Author

I'm sorry, no.
I will be able to look at this this week, but not before Wednesday or Thursday

@ckiss
Copy link
Contributor

ckiss commented Feb 9, 2016

@paolochiodi no worries. This week is fine. thanks

@mihaidma
Copy link
Contributor

@paolochiodi @ckiss i started working today on the mysql plugin. I will fix also this PR

@paolochiodi
Copy link
Collaborator Author

thanks @mihaidma

@mcdonnelldean
Copy link
Contributor

@mihaidma @paolochiodi What's the story with this PR? is it still valid? It needs resolving if so.

@mihaidma
Copy link
Contributor

@mcdonnelldean I am currently working on postgres but by tomorrow I will switch to mysql. Mysql will use seneca-standard-query for query generation and all this PR will be probably cancelled. I will not close it until I look over the fixes not to lose any info.

@mcdonnelldean
Copy link
Contributor

@mihaidma awesome stuff 👍

@mihaidma
Copy link
Contributor

the mysql was unified with the postgres one (and reusing the postgres code), this won't be merged as the code is not the same anymore

@mihaidma mihaidma closed this Apr 28, 2016
@mihaidma mihaidma deleted the reorganize branch August 4, 2016 07:49
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.

5 participants