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

Remove duplicate ops from lib.rs. #2272

Merged
merged 10 commits into from
Nov 4, 2024
Merged

Remove duplicate ops from lib.rs. #2272

merged 10 commits into from
Nov 4, 2024

Conversation

jku20
Copy link
Collaborator

@jku20 jku20 commented Aug 27, 2024

This PR removes ops from lib.rs which are duplicated in Rhai scripts.

@jku20 jku20 marked this pull request as ready for review August 28, 2024 04:02
@jku20
Copy link
Collaborator Author

jku20 commented Sep 4, 2024

@nathanielnrn I think you're code is the only bit of rust code which is still needed. I've tried to port it over but it seems to fail tests, but I suspect this is because it simply has a different output!

Can you verify the behavior in this commit is what's intended? My sed is not good enough to confidently figure out what the addition does and reason about it myself.

@jku20
Copy link
Collaborator Author

jku20 commented Sep 16, 2024

So, just removed the new sed command. I think (in part based on the comment and what it was doing) it was just kind of cleaning up a file to make it a bit more readable.

I don't think that codepath was being taken in the current tests (which I think go through Rhai entirely right now).

So yeah, I'm of mind to just go ahead and remove the lib.rs now!

@rachitnigam
Copy link
Contributor

@jku20 are we planning to merge this? If not, what's blocking us?

@jku20
Copy link
Collaborator Author

jku20 commented Oct 7, 2024

Nothing's blocking this other than a review and me forgetting to ask someone for one.

@jku20
Copy link
Collaborator Author

jku20 commented Oct 18, 2024

Okay, so this is broken still because I think it will die if it compiles without "migrate_to_scripts" set. I think the course of action is to fix that and then just merge it (hopefully like this weekend or something like that).

@rachitnigam
Copy link
Contributor

@jku20 can you say a bit more what this mean? I would like to get this merged.

@jku20
Copy link
Collaborator Author

jku20 commented Nov 3, 2024

Currently lib.rs defines build_driver which I removed. This means when migrate_to_scripts isn’t set, the fud2 doesn’t compile. The fix is as simple as removing the compile option and all the references to build_driver. I should really just do that today (haven’t gotten around to it because had a pretty hectic couple weeks).

@jku20
Copy link
Collaborator Author

jku20 commented Nov 3, 2024

Should be ready to be merged!

Copy link
Contributor

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Looks perfect!! Thank you, @jku20!

@sampsyo sampsyo merged commit 3687b07 into main Nov 4, 2024
18 checks passed
@sampsyo sampsyo deleted the fud2-remove-duplicate-ops branch November 4, 2024 00:55
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.

3 participants