Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Updated oniguruma dependency to support node.js 6 #74

Merged
merged 2 commits into from
Aug 10, 2016
Merged

Updated oniguruma dependency to support node.js 6 #74

merged 2 commits into from
Aug 10, 2016

Conversation

janb87
Copy link
Contributor

@janb87 janb87 commented May 26, 2016

Fixes #72

@@ -6,6 +6,8 @@ notifications:
on_failure: change

node_js:
- 6
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we always test the latest Node version, eg using node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would just list the versions explicitly. Most projects do it like that.

@zeke
Copy link
Contributor

zeke commented Jun 11, 2016

@50Wliu I see you added the needs-testing label. What testing need to be done before this can be shipped?

@winstliu
Copy link
Contributor

Just some basic regression testing should suffice, though you probably know more about what needs to be done than I do :).

@zeke
Copy link
Contributor

zeke commented Jun 11, 2016

oniguruma doesn't seem to have a changelog or release notes.

Hey @zcbenz, @alexandrudima, and @evanlucas: do you know what breaking change happened in oniguruma from 5 to 6?

https://github.com/atom/node-oniguruma/commits/master

@alexdima
Copy link
Contributor

@zeke: atom/node-oniguruma#46

And the relevant comment:

// This is the only way to get caching between `FindNextMatchSync` calls:
var str = new OnigString("hello world");
scanner.FindNextMatchSync(str, 0);
scanner.FindNextMatchSync(str, 1);
scanner.FindNextMatchSync(str, 2);
...

To benefit from caching between calls on the same scanner instance you need to create a OnigString up-front. I explain the reason behind that in the PR.

@ashleygwilliams
Copy link

hey ya'll- what can i do to help move this forward?

@zeke zeke merged commit 475bf89 into atom:master Aug 10, 2016
@zeke
Copy link
Contributor

zeke commented Aug 10, 2016

Just landed this in a new major version: 6.0.0

@ashleygwilliams
Copy link

zomg amazing!! thank you so much! 🎉

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

Successfully merging this pull request may close these issues.

6 participants