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.

Sign up for GitHub

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

Jump to bottom

Minor Improvements and Issue #253 #259

Closed
wants to merge 5 commits into from
Closed

Minor Improvements and Issue #253 #259

wants to merge 5 commits into from

Conversation

TonyRice
Copy link
Contributor

This pull requests includes some minor improvements and a version bump to 7.10.0 in the tests. I have built against node version 7.10.0 with all tests passing.

@TonyRice TonyRice mentioned this pull request May 16, 2017
Closed
@irbull
Copy link
Member

irbull commented May 23, 2017

I like some of these changes. However, I don't understand the commit bump version to 7.10. The .travis.yml and the newly created docker build scripts download a prebuilt node 7.4. We need to create a 7.10 and update all the build scripts to use that. I'm happy to start down that path, but since it took over 1 month to get J2V8 building against 7.4, let's stick with that for now and get a release out.

irbull pushed a commit that referenced this pull request May 25, 2017
@TonyRice @irbull
Add a generic push(Object object) to V8Array
e18164a 
To allow V8Arrays to be constructed more easily, add a generic
push(Object object) method to V8Array. Push will inspect the type
of object being used, and properly add that object to the underlying
V8Array.

If a non-compatible Object is passed to push, then an
IllegalArgumentException is thrown.

This was originally contributed by @TonyRice in PR #259.

Change-Id: I193bf99a29283899f7315a28c1fe989edd87eb1e
@irbull
Copy link
Member

irbull commented May 25, 2017 •
edited
Loading

I have pulled some of this work into the master branch. I have left out the update to node 7.10 since upgrading the version of node.js is a lot more work than updating a single test case. We are still working towards an easier build (See #232) and until we have this done, I don't think we should upgrade the version of node. Let's get a release out first :).

I have pulled other other changes. In particular, I have cherry-picked the additional registration of windows methods (43a66da), Used the full name of the lib in the library loader (c8efb4a), And added the new push(Object object) method (e18164a). I have consolidated the two commits that @TonyRice did and added a number of test cases.

Since this work is now in, I'm going to close this PR. Thanks @TonyRice.

@irbull irbull closed this May 25, 2017
@TonyRice TonyRice deleted the my_changes branch May 27, 2017 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TonyRice @irbull