-
Notifications
You must be signed in to change notification settings - Fork 31
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
Finish provider re-organization #15
Comments
Hi @thepumpkin1979 isn't that what Joe's 2nd commit updates (at least a good part of it)? |
I didn't see that, Yeah, you can merge that. The localfs module was uploaded to NPM, please test it to see if it's working property. |
Should I change the package.json to version 0.1.0 before merging this major change? |
Merge and then comes the Version Bump :) I'm not saying that we have right now(my bad), but the Workflow should be:
Notes:
What do you think of this? |
Please have a look - and merge if it's ok. |
what else do we need? are the other modules ready to be published? |
Yes, I would say so. What about #11 - it includes the necessary changes to the README and removes the provider files. Can't we simply merge that? (You just closed it without merging?) Concerning heapsource/mongoose-attachments-localfs#4 - I would rather not merge that one. It's supposed to be "local fs", at least for now, and this issue is about problems when copying images from one device to another. |
Ok, after checking the package.json files of the provider modules:
mongoose-attachments-knox and mongoose-attachments-aws2js are not on npm, yet. |
... and the provider name should be "localfs" or "fs" depending on whether the module is mongoose-attachments-localfs or mongoose-attachments-fs. (Currently, it's "fs".) Wich one do you prefer? It's messed up as it is, anyway: npm knows about "mongoose-attachments-fs" but the github module is "mongoose-attachments-localfs"? Sorry :-( |
I can republish with -force and reset the tag, no problem. Choose whatever name you'd like and let me know what I need to change. Don't worry, If we need to re-publish then we do it. |
I think local-fs is ok since the repo name is already local-fs, I don't know, we can still rename it I guess |
In the readme, let's include links to the other providers. |
btw, the readme still says |
Ok, I will change localfs to be localfs everywhere in mongoose-attachments-localfs and make a pull request for that. For the decision on how to require the providers - whether they return the attachments instance right away, I'd rather have a different branch. (See my comment in #17 ) The package.json of mongoose-attachments still says 0.0.4 but I didn't want to just change that as part of some commit. |
How are we with this reorganizacion @nuarhu ? |
The pull request #17 removes the providers and updates the README. I didn't want to merge it without you having a look at it. Can you check and merge? There is still to decide whether the provider submodules should return a reference to mongoose-attachments. (See the comment in the pull request.) Depending on that, there is a little work to be done in the submodules. |
Hi @thepumpkin1979 , sorry for not working on this for so long. I hope to have some spare time this week to finish this, and maybe have a look at the issues that are coming in. |
Ready for release from my part. |
In order to reflect recent changes, We need to:
The text was updated successfully, but these errors were encountered: