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

Consider moving from js-yaml to yaml module #243

Closed
MikeRalphson opened this issue Jan 6, 2019 · 11 comments
Closed

Consider moving from js-yaml to yaml module #243

MikeRalphson opened this issue Jan 6, 2019 · 11 comments

Comments

@MikeRalphson
Copy link
Contributor

MikeRalphson commented Jan 6, 2019

Detailed description

js-yaml has some bugs related to the YAML 1.2 specification, in that it appears to convert strings which contain only digits and begin with a 0 into octal integers when it should not. This may only be the case where the {json:true} option has been provided. It also seems to drop seconds/fractions of a second from date-time string properties.

The yaml module is tested against the official yaml-test-suite and is smaller (not including the esprima engine). It has far fewer open issues (but may be much less utilised in the wild so far). It also has the ability to build an abstract-syntax-tree or concrete-syntax-tree built in. It will also hopefully be gaining some nice features such as:

(if my PRs are accepted).

Context

oas-kit is going to migrate so node_modules de-duplication will result in an even smaller installed size. This will probably be a breaking change in oas-kit (all version numbers bumped by a major revision).

Possible implementation

Replace js-yaml with yaml, safeLoad with parse, safeDump with stringify.

Your environment

all

More...

This issue is a heads-up really. But I'm happy to PR the changes.

@dlouzan
Copy link
Contributor

dlouzan commented Jan 10, 2019

Edit: comment moved to #249

@MikeRalphson
Copy link
Contributor Author

@dlouzan sounds very similar to a new issue #249 - maybe we can move the discussion there?

@philsturgeon
Copy link
Contributor

I'll do this when #250 is finished (adding better testing to the command files).

@pderaaij
Copy link
Contributor

@MikeRalphson yoou mentioned working on the yaml module, but you meant the yaml package right? Not changing speccy to make use of yaml.

@MikeRalphson
Copy link
Contributor Author

Yes, on yaml itself, not on moving Speccy from js-yaml to yaml.

@pderaaij
Copy link
Contributor

Clear, I'll have a look at that in the next few days

pderaaij added a commit to pderaaij/speccy that referenced this issue Jan 23, 2019
@pderaaij
Copy link
Contributor

Okay, few days became this hour ;-)

@dlouzan
Copy link
Contributor

dlouzan commented Mar 14, 2019

I lost track of the status of this, is it now possible to get proper line numbers on errors when validating yaml? Today I found some issues with yaml comments, and it seems the latest master doesn't still include this functionality.

@eemeli
Copy link

eemeli commented Mar 14, 2019

Line/column locations should be available in yaml at error.source.rangeAsLinePos, starting from its version 1.3.

@dlouzan
Copy link
Contributor

dlouzan commented Mar 15, 2019

Ok, I see that #267 is still open, and uses 1.2.

@philsturgeon @MikeRalphson How easy would be to integrate 1.3 and modify the error reporting to get meaningful reports? I'm not familiar with that part of the code (I'm not even sure if this would need to be modified in oas-kit).

@djtarazona
Copy link
Contributor

This has been addressed in v0.10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants