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

Windows node-jq and jq fix #72

Merged
merged 3 commits into from Dec 29, 2016
Merged

Windows node-jq and jq fix #72

merged 3 commits into from Dec 29, 2016

Conversation

MaanooAk
Copy link
Contributor

@MaanooAk MaanooAk commented Dec 29, 2016

  • node-jq v.0.5.0 now supports windows
  • jq has a limit to the length of the arguments, I changed the path of the tmp file from absolute to relative (= smaller) and it works.

This pr resolves all jq issues in Windows (#56 and others?)

@@ -70,6 +70,8 @@ class Editor extends EventEmitter {

// Path to store data file so it can be read for jq (required for large input)
this.tmp = path.resolve(__dirname, '..', 'tmp', 'data.json')
Copy link
Owner

Choose a reason for hiding this comment

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

Is this needed anymore?

@@ -249,7 +251,7 @@ class Editor extends EventEmitter {
fs.writeFileSync(this.tmp, JSON.stringify(this.data))
Copy link
Owner

@wellsjo wellsjo Dec 29, 2016

Choose a reason for hiding this comment

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

We could prob use the same tmpRelative path here. Let's change tmpRelative back to to tmp though if we delete the other one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@wellsjo
Copy link
Owner

wellsjo commented Dec 29, 2016

LGTM 👍

@wellsjo wellsjo merged commit 7dacdf4 into wellsjo:master Dec 29, 2016
@wellsjo
Copy link
Owner

wellsjo commented Jan 4, 2017

@MaanooAk this ended up causing it not to work on Mac. we need to find a better solution, or at least something that separates the tmp path for both operating systems.

@wellsjo wellsjo mentioned this pull request Jan 4, 2017
@MaanooAk
Copy link
Contributor Author

MaanooAk commented Jan 4, 2017

I have no experience with mac, I am sorry I can't help (or test)... but using the old way for mac and the new way for everything else should be easy to implement. Can you test on mac?

@wellsjo
Copy link
Owner

wellsjo commented Jan 4, 2017

@MaanooAk Yeah I will try that today. Been really busy lately so I didn't even notice.

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.

None yet

2 participants