Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Several updates #137

Closed
wants to merge 9 commits into from
Closed

Several updates #137

wants to merge 9 commits into from

Conversation

jy95
Copy link

@jy95 jy95 commented Mar 3, 2017

Summary:

  • Ability to select the errorHandler language more easilly than ever (and added in documentation)
  • Update RAML module dependencies (all from mulesoft)
  • Update out of date dependencies (yargs , finalhandler and form-data)

Why:
Probably fix a lot of parsing and another hidden issues XD

sources:

https://github.com/mulesoft-labs/osprey-router/releases/tag/v0.5.0
https://github.com/raml-org/raml-js-parser-2/releases/tag/1.1.19
https://david-dm.org/mulesoft/osprey
...

RAML 1.0 support
@coveralls
Copy link

coveralls commented Mar 3, 2017

Coverage Status

Changes Unknown when pulling 300fb41 on jy95:master into ** on mulesoft:master**.

@jy95 jy95 mentioned this pull request Mar 3, 2017
request-error-handler allow to select the message error language, why not use it for osprey ?
@coveralls
Copy link

coveralls commented Mar 3, 2017

Coverage Status

Changes Unknown when pulling 477210c on jy95:master into ** on mulesoft:master**.

@jy95 jy95 changed the title Update osprey-router to 0.5.0 Update osprey-router to 0.5.0 and Ability to select the errorHandler language Mar 3, 2017
@jy95
Copy link
Author

jy95 commented Mar 3, 2017

I just added the ability to select the message error language : It could be useful for translation
(since https://github.com/mulesoft-labs/node-request-error-handler can handle another languages than english)

Added defaultLanguage option in README.md
@coveralls
Copy link

coveralls commented Mar 3, 2017

Coverage Status

Changes Unknown when pulling e1a3268 on jy95:master into ** on mulesoft:master**.

@jy95 jy95 changed the title Update osprey-router to 0.5.0 and Ability to select the errorHandler language Several Update Mar 13, 2017
Update raml-1-parser to 1.1.17
@jy95 jy95 changed the title Several Update Several updates Mar 13, 2017
@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

Changes Unknown when pulling d65c972 on jy95:master into ** on mulesoft:master**.

@jy95
Copy link
Author

jy95 commented Mar 22, 2017

no comment about this PR ?

@jstoiko
Copy link
Contributor

jstoiko commented Mar 22, 2017

Thanks for your contribution! Sorry for not getting back earlier.

AFAICT from the README, it seems like "all options are overridable" already: https://github.com/mulesoft/osprey/blob/master/README.md#error-handler

Have you tried passing an options.errorHandler?

@jy95
Copy link
Author

jy95 commented Mar 23, 2017

Yes but It was something like this :

let ospreyOptions = {errorHandler: responder};
...
osprey.loadFile(path, ospreyOptions)

and my custom responder :

// Error Handler for osprey
function responder(req, res, error, stack) {
    /* Custom respond logic here */
    let message = "There have been validation errors";
    res.status(400).json({
        statusCode: 400,
        message: message,
        requestErrors: error || []
    });
}

In this version, passing the language argument is unclear.

From what I have read : the standard practice is to install all of your dependencies explicitly. That is why I thought this update enables easily changes inside the ospreyOptions (and maybe in the future will be used for another dependencies that osprey use)

let ospreyOptions = {errorHandler: responder, defaultLanguage : "en"};
...

Update raml-1-parser to 1.1.18
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 72895dd on jy95:master into ** on mulesoft:master**.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 72895dd on jy95:master into ** on mulesoft:master**.

@jy95
Copy link
Author

jy95 commented Mar 27, 2017

I don't know if you have seen this but with the latest version of the parser, the code coverage that is at 94% with the currently with the latest osprey release. With my update : 97%.

@jstoiko
Copy link
Contributor

jstoiko commented Mar 27, 2017

Not sure how coverall calculates that delta. The PATCH increment in the parser version of package.json shouldn't matter since we use carets: https://docs.npmjs.com/misc/semver#caret-ranges-123-025-004

@jy95
Copy link
Author

jy95 commented Mar 27, 2017

... Do you see some bugfixes on the changelog of this parser that seems to increase the coverage then ?

I say this because I also use several tools related to raml (for example : raml2html that rely on raml2obj that use raml-1-parser to 1.1.18)
They have fixed some issues like this : raml-org/raml-js-parser-2#663

Update dependencies to the latest known
@coveralls
Copy link

coveralls commented Apr 8, 2017

Coverage Status

Changes Unknown when pulling 153f03b on jy95:master into ** on mulesoft:master**.

@coveralls
Copy link

coveralls commented Apr 8, 2017

Coverage Status

Changes Unknown when pulling 4200543 on jy95:master into ** on mulesoft:master**.

@jy95
Copy link
Author

jy95 commented Apr 8, 2017

I was curious and I found out that 3 dependancies were out of date :
yargs , finalhandler and form-data ...

I updated them to the latest stable version. Hope it will help you (tests passed).

@jy95
Copy link
Author

jy95 commented Apr 27, 2017

this PR will never be merged ?

@jstoiko
Copy link
Contributor

jstoiko commented Apr 28, 2017

sorry @jy95, I haven't gotten around to review this before the latest release. I'll do my best to include it in the next patch release. Thanks for your contribution!

Update raml-1-parser to 1.1.20
@jy95
Copy link
Author

jy95 commented May 2, 2017

A new release of the parser is out : https://github.com/raml-org/raml-js-parser-2/releases/tag/1.1.20 . The greatest bug fix I see in this release : raml-org/raml-js-parser-2#713

Restore version with carret
@coveralls
Copy link

coveralls commented May 2, 2017

Coverage Status

Changes Unknown when pulling 618646f on jy95:master into ** on mulesoft:master**.

@jy95
Copy link
Author

jy95 commented May 9, 2017

Just to notice you, The parser has a new release : https://github.com/raml-org/raml-js-parser-2/releases/tag/1.1.21. It fixes this issue : raml-org/raml-js-parser-2#729 (the performance)

@jstoiko
Copy link
Contributor

jstoiko commented May 10, 2017

I appreciate the update @jy95 however note that changing the PATCH version (i.e. 1.1.n) of raml-js-parser in the package.json of osprey won't do much since we're using caret range versioning.

@jy95
Copy link
Author

jy95 commented May 10, 2017

I know .. It was a notice (so do whatever you want with this ^^). What about the out-of-date dependancies I told you ?

@jstoiko
Copy link
Contributor

jstoiko commented May 10, 2017

What about the out-of-date dependancies

thanks for doing this. this can wait for the next release unless you tell me that something is wrong with one of the dependencies that requires immediate attention

@jy95
Copy link
Author

jy95 commented May 10, 2017

I don't think there is something wrong. The most critical is form-data that is in rc release ..

@jy95 jy95 closed this Jan 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants