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

ENH: Remove lua bindings #37

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

ENH: Remove lua bindings #37

wants to merge 20 commits into from

Conversation

RuhiRG
Copy link
Member

@RuhiRG RuhiRG commented Jul 17, 2023

In keeping with #36. Originally this PR was meant to only change the function return types. However, as noted below, this breaks the CI.

So it was later further expanded to completely remove the lua bindings, since they are incompatible with the new by value return policies of the functions.

Closes #35 as well.

@RuhiRG RuhiRG requested review from amritagos and HaoZeke as code owners July 17, 2023 23:24
Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a ton @RuhiRG

Comment on lines 690 to +689
<< "Fatal Error: The file does not exist or you gave the wrong path.\n";
// Throw exception?
return *yCloud;
return yCloud;
Copy link
Member

Choose a reason for hiding this comment

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

This should actually throw. Not for this PR though :)

Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

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

The test failures might be because the lua bindings need some work.

@RuhiRG RuhiRG force-pushed the fixup branch 2 times, most recently from 0693ba0 to 4c7f9f8 Compare August 23, 2023 16:03
@HaoZeke HaoZeke changed the title ENH: changed the return type for readLammpsTrjreduced function ENH: Remove lua bindings Aug 23, 2023
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.

ENH,BIND: Return more PointClouds
2 participants