-
Notifications
You must be signed in to change notification settings - Fork 181
add base support for exporting objects and classes in node_loader #343
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
base: develop
Are you sure you want to change the base?
Conversation
1a5663b to
53be583
Compare
6fb9758 to
1334267
Compare
00bc5a8 to
22e43f3
Compare
2678438 to
67e344f
Compare
|
Last commits look pretty well, I recommend you to implement tests in order to verify the behavior. TDD is always easier to develop. Check out |
|
Check out the tests, no one is passing, try to pass them before pushing. |
e44e0ab to
f6078f4
Compare
I just figured out what's making the tests fail. I would fix it as soon as possible |
add api for getting all keys and values in adt_set and adt_map
|
I node port it seems there's a bug:
|
|
Also there's this in windows:
But I think this has been already solved in develop, can you merge develop into your branch and push so we can see if that test passes? |
My branch is up-to-date. I don't run windows so I can't really debug the windows test |
That's not the only problem. There's alot of load_from_file errors... look at this Where should |
Those errors are fine. I am reviewing everything, you overwrote all my changes. I am going to try to solve them. |
|
I think I almost get it to work, I want still to fully review it locally. |
|
Yeah, it seems all tests passed, but I still want to fully review it. Formatting failed because I modified it from the web, but I also want to fully review all the implementation properly, I've seen things that are not clear to me and I need to understand them better and provide better feedback to you. Good job! |
|
@rxbryan hey mate I am going to review all this and probably reimplement it using parts of your PR, I prefer doing that because it's the most effective way of reviewing it in depth, specially for debugging complex memory issues or similar. Once I finish it I will co-author the PR with you so you get credits of the task. This is the last step towards making NodeJS fully interoperable between Python and Ruby. |
|
Hi @viferga
Iooking forward to that. |
Description
add base support for exporting objects and classes in node_loader
Fixes #(issue_no)
Type of change
Checklist:
make testorctest -VV -R <test-name>)../docker-compose.sh build &> outputand attached the output.OPTION_BUILD_SANITIZERor./docker-compose.sh test &> outputandOPTION_TEST_MEMORYCHECK.Helgrindin case my code works with threading.make clang-formatin order to format my code and my code follows the style guidelines.If you are unclear about any of the above checks, have a look at our documentation here.