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

[WIP] Support the standard JSON via compile() too #122

Closed
wants to merge 2 commits into from
Closed

Conversation

axic
Copy link
Member

@axic axic commented Jul 18, 2017

@axic
Copy link
Member Author

axic commented Jul 18, 2017

Needs tests.

@axic axic force-pushed the better-wrapper branch 2 times, most recently from 62a6309 to 1597b73 Compare July 18, 2017 14:45
@axic axic changed the title Support the standard JSON via compile() too [WIP] Support the standard JSON via compile() too Jul 25, 2017
@axic axic force-pushed the better-wrapper branch from 1597b73 to 89d01ee Compare July 25, 2017 20:14
@axic axic force-pushed the better-wrapper branch 4 times, most recently from b7549b0 to e8ece8b Compare February 21, 2018 15:30
@coveralls
Copy link

coveralls commented Feb 21, 2018

Coverage Status

Coverage increased (+0.09%) to 58.355% when pulling 38db1f7 on better-wrapper into 24431b8 on master.

@axic axic requested a review from chriseth March 7, 2018 09:01

if (isStandardJSON) {
// NOTE: takes second argument as "readCallback"
return JSON.parse(compileStandardWrapper(input, optimise));
Copy link
Contributor

Choose a reason for hiding this comment

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

compileStandardWrapper is only assigned in line 83 / 105

Copy link
Member Author

Choose a reason for hiding this comment

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

Javascript scoping... 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it's a callback!

Copy link
Contributor

@chriseth chriseth left a comment

Choose a reason for hiding this comment

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

I am not convinced this is working.

@axic
Copy link
Member Author

axic commented Mar 7, 2018

I am not convinced this is working.

Probably it isn't yet, it is more about discussing the API.

Eventually at 0.5.0 I'd like to replace compile to only do what compileStandardWrapper is doing. This meant to be a stepping stone to there.

@chriseth
Copy link
Contributor

chriseth commented Mar 7, 2018

Yes, I think it is a good idea to make compile to also take json-io.

@axic
Copy link
Member Author

axic commented Apr 13, 2018

This needs tests.

@@ -104,6 +104,19 @@ There is also a direct method, `compileStandard`, which is only present on recen

Starting from version 0.4.20 a Semver compatible version number can be retrieved on every compiler release, including old ones, using the `semver()` method.

#### From version 0.4.21
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh man, only if we'd have merged this at 0.4.21, it wouldn't be too big a breaking change anymore :)

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.

3 participants