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

Bugfix/agcmd 1792 #64

Merged
merged 13 commits into from
Feb 22, 2016
Merged

Bugfix/agcmd 1792 #64

merged 13 commits into from
Feb 22, 2016

Conversation

ShurakaiSoft
Copy link
Contributor

Updated hapi to v13.0.0 which also required updating hapi-swagger to v4.2.1.

})
it('should describe models', function () {
// not sure this data exists...
it.skip('should describe models', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So new happi-swagger does not include models anymore?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that /docs contains definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw definitions too. Didn't look too closely as the start of that contained seemingly random data. However at the bottom there is some useful stuff, we can look at, to try to recreate this test.
Since rebasing this work was a bit painful, I'm going to create a new ticket to fix this test so this can be merged without conflict now.

@@ -58,7 +69,7 @@ describe('Swagger docs', function () {
require('inject-then'),
require('inert'),
require('vision'),
{register: require('hapi-swagger'), options: {apiVersion: require('../package.json').version}}
{ register: HapiSwagger, options: swaggerOptions }
], () => {
server.start(() => {
_.forEach(schemas, function (schema) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is not your code, but there is a simpler way to register the routes

 _.each(harvester.routes.all(schema.brands), (route) => server.route(route))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. I'll make the change. After all any improvement to general code quality is worth doing.

kristofsajdak added a commit that referenced this pull request Feb 22, 2016
@kristofsajdak kristofsajdak merged commit e626fab into develop Feb 22, 2016
@kristofsajdak kristofsajdak deleted the bugfix/AGCMD-1792 branch February 22, 2016 20:02
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.

4 participants