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

Fix blueprints puralize option #3223

Merged
merged 3 commits into from
Dec 23, 2015
Merged

Conversation

armellarcier
Copy link
Contributor

don't call pluralize('/quiz') (returns /quizs). call pluralize('quiz') (returns quizzes).

don't call `pluralize('/quiz')`. call `pluralize('quiz')`.
@listepo
Copy link
Contributor

listepo commented Sep 12, 2015

+1

@tjwebb
Copy link
Contributor

tjwebb commented Sep 13, 2015

@Benew mind including a couple unit tests?

@sylvainlap
Copy link
Contributor

Tests on pluralized routes already exists:

https://github.com/balderdashy/sails/blob/master/test/integration/router.APIScaffold.test.js#L186

and they pass.

@armellarcier
Copy link
Contributor Author

@tjwebb what should we do?

@mikermcneil
Copy link
Member

Tests on pluralized routes already exists:

https://github.com/balderdashy/sails/blob/master/test/integration/router.APIScaffold.test.js#L186

and they pass.

Please add tests that demonstrate the breaking behavior (i.e. that fail when your patch is not in place). Thanks!

@armellarcier
Copy link
Contributor Author

This is weird... The tests are failing because of an unrelated timeout on node v0.12

@armellarcier
Copy link
Contributor Author

Thanks @sylvainlap ! Looking forward to the merge!

@armellarcier
Copy link
Contributor Author

When can we hope for a merge?

Btw, would be more than happy to contribute more since some anchors are leaving the ship ;-)

@tjwebb
Copy link
Contributor

tjwebb commented Dec 7, 2015

@Benew we're leaving to work on Trails.js: https://github.com/trailsjs. We'd love to have your help over there. No reason to try to catch a falling knife.

@armellarcier
Copy link
Contributor Author

@tjwebb So you think I should wait and move my project to trails? What's the roadmap?
I think a lot of us will want to keep using sails for production apps. And some of us might want to maintain sails. Don't go without leaving the keys under the mat...

@tjwebb
Copy link
Contributor

tjwebb commented Dec 7, 2015

I think a lot of us will want to keep using sails for production apps. And some of us might want to maintain sails. Don't go without leaving the keys under the mat...

The next "version" of Sails will actually be called Trails.js, and there will be an upgrade path. Sails is a legacy platform. Express3 is EOL, and there's many fundamental problems in the Sails framework. Since there will be an upgrade path, feel free to keep using it in the meantime, but I wouldn't invest too much time in improving the Sails framework itself.

@sgress454
Copy link
Member

Thanks @Benew, sorry we're just getting around to this. Rest assured that Sails is under active development and we have no plans to stop! Travis is working on his own framework now, but he does not speak for the Sails core team, and his framework is by no means the "next version" of Sails. We're currently in the process of going over all of the merged PRs since the last release to make sure things are stable for v0.12.

Re: Express 3, we're using it because it's stable and tested with Sails, and we're committed to maintaining any issues that arise with it until we are able to follow through on our plan to separate the router out entirely, at which point you'll be able to use Express 4 or whatever other backing server you desire (see #3235 (comment)).

This PR looks good, just running the tests and I'll merge. Thanks for your patience.

@sgress454 sgress454 merged commit 7edebb4 into balderdashy:master Dec 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants