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

Add support for additional file types via any-json #66

Merged
merged 3 commits into from
May 11, 2020
Merged

Add support for additional file types via any-json #66

merged 3 commits into from
May 11, 2020

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Oct 28, 2019

It all works pretty well considering; I've adjusted it so that it shouldn't be a breaking change - i.e you can still use a javascript file.

Not too happy about deasync-promise, since it's binary package, but I think this a good first step.

To eliminate it, ajv-cli would need to be refactored to use promises - it's not a big change, but it's a knock on effect that requires the whole general chain of methods to be touched, so I'd prefer to get a thumbs up before sinking time into.

Resolves #61

@coveralls
Copy link

coveralls commented Oct 28, 2019

Coverage Status

Coverage increased (+0.9%) to 95.572% when pulling 46b3fd3 on G-Rath:use-any-json-to-support-other-file-types into d9de674 on jessedc:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-26.1%) to 68.519% when pulling b9ceb25 on G-Rath:use-any-json-to-support-other-file-types into aba2520 on jessedc:master.

@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 28, 2019

@jessedc @epoberezkin I'm happy to give this library some love if you'd like :)

I've also got a package-lock.json locally that I can commit & push if you'd like.


Weird; pretty sure coverage was already at least 20% down before adding this :/

@epoberezkin
Copy link
Member

Hey - this package is definitely happy to get more love than it’s getting from me.

I will check what’s happening here with the coverage. It may be that coveralls changed the way it’s counted if there were no large changes?

@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 31, 2019

@epoberezkin cheers - let me know what you find, or if there's anything I can do to help.

this package is definitely happy to get more love than it’s getting from me.

Awesome, I"ll put this on my list of things to look into when I get some bandwidth.

Personally I'm quite keen on this library as I work at a primarily ruby & php heavy shop, so being able to have a line like npx <library> <json> <schema> in our CI is a huge win vs having to always have another package manager in the repo, and so makes it easy to justify devoting some bandwidth to maintain :)

@G-Rath
Copy link
Contributor Author

G-Rath commented Nov 7, 2019

@epoberezkin I'd say something def has changed w/ coveralls, as I just opened this PR oclif/config#90 (comment) moving a package from devDependencies to dependencies, and it apparently decreases the coverage by 65% 😂

@G-Rath
Copy link
Contributor Author

G-Rath commented Dec 3, 2019

@epoberezkin have you had a chance to look at what's causing coverage problems?

Would be cool to get this out the door so we can start the love giving & move off pajv :D

@G-Rath G-Rath mentioned this pull request Dec 7, 2019
@epoberezkin
Copy link
Member

tests pass, needs to be covered in docs.

@G-Rath G-Rath requested a review from epoberezkin February 22, 2020 23:19
@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 21, 2020

@epoberezkin light ping - I know you're pretty busy, but would be great to get this across so I can start giving further love :)

@epoberezkin
Copy link
Member

epoberezkin commented Apr 22, 2020

@G-Rath I like the idea of supporting multiple formats, but while (!done) loop() (in deasync) looks like a code smell to me to convert from async to sync code....

There are quite a few synchronous parsers that parse these formats - yml probably being the most important anyway - I would rather we use them then any-json that is for some reason asynchronous.

Another viable option is to refactor the code to async and use promises directly. But using synchronous parsers seems better in this case.

Copy link
Member

@epoberezkin epoberezkin left a comment

Choose a reason for hiding this comment

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

see comment

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 22, 2020

looks like a code smell to me to convert from async to sync code

that's exactly what it's doing: from memory, it's exploiting the fact that C++ libs are allowed to say if they're sync or async, and so uses a lib binding to say "take an async promise, block, and return its results as sync".

I'm not a fan of it either, but since the feature worked as-is I aimed for getting it landed before doing any refactoring that might change behaviour; but since you're on board with having a larger PR I'm more than happy to look to refactor it to be sync.

(This ties in well with moving off any-json anyway as the maintainer has passes, and it's got no other owners attached that could publish new versions as far as I know).

@epoberezkin
Copy link
Member

I'd rather we just use sync parser for yaml to be honest, other formats are not that needed.

@G-Rath
Copy link
Contributor Author

G-Rath commented May 9, 2020

@epoberezkin I've reimplemented this PR to use the js-yaml & json5 parsers, which I think are the two main parsers worth supporting (vs complexity to implement them).

The others either required more complex implementation (i.e ini had some array conditional checking), used promises (i.e csv), or were overly large (i.e cson, which was 80kb and ships with a cli that handles converting to json, so you should be able to just pipe the two together)

@epoberezkin epoberezkin merged commit 09382a0 into ajv-validator:master May 11, 2020
@epoberezkin
Copy link
Member

Great, thank you, Gareth!

@PascalPflaum
Copy link

@epoberezkin or @jessedc, thanks for this great repo. Would you mind bringing this version to npm? I would really like to use this feature.

@epoberezkin
Copy link
Member

@PascalPflaum will do

@epoberezkin
Copy link
Member

released

@G-Rath G-Rath deleted the use-any-json-to-support-other-file-types branch June 1, 2020 07:41
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.

Consider adding additional formats (from the pajv fork)
4 participants