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

Ruby bindings for existing FFI methods, plus eval_rule() #188

Merged
merged 5 commits into from
Apr 2, 2024

Conversation

thedavemarshall
Copy link
Contributor

@thedavemarshall thedavemarshall commented Mar 29, 2024

I have a few things left to figure out, but I wanted to get some early feedback on my first time wring Rust. Feedback is welcome!

  • improve the Ruby tests
  • add the Ruby bindings to the github workflows CI/CD

The other thing I'd call out is that I ran into some issues with a circular build when I had the bindings/ruby/ext/regorusrb/Cargo.toml depend on the relative location of regorus, so I locked it to 0.1.2 . Other language bindings seem to work with the relative paths, I think this might be something to do with the other manifest at bindings/ruby/Cargo.toml? Not sure if resolving this is a blocker or if we can manually update the versions as needed for each release.
Update- I think I got it to work with relative paths for the crate!

The other callout is that rubygems expects a Cargo.lock file to be included, source, which in this case is the top level regorus crate's Cargo.lock

@thedavemarshall
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Collaborator

@anakrish anakrish left a comment

Choose a reason for hiding this comment

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

LGTM overall.
I don't quite follow the Ruby details; but will try it out locally.
The PR title could be updates as well.

bindings/ruby/README.md Outdated Show resolved Hide resolved
bindings/ruby/README.md Outdated Show resolved Hide resolved
bindings/ruby/README.md Outdated Show resolved Hide resolved
bindings/ruby/ext/regorusrb/src/lib.rs Show resolved Hide resolved
bindings/ruby/ext/regorusrb/src/lib.rs Outdated Show resolved Hide resolved
bindings/ruby/ext/regorusrb/src/lib.rs Show resolved Hide resolved
@anakrish
Copy link
Collaborator

Looks great, especially if this is the first time with Rust!

…DME.md

also added rubocop-minitest and rubocop-rake, and added more test coverage
@thedavemarshall thedavemarshall changed the title Ruby bindings against regorus 0.1.2 Ruby bindings for existing FFI methods, plus eval_rule() Mar 31, 2024
Copy link
Collaborator

@anakrish anakrish 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 for this contribution!

@@ -0,0 +1,180 @@
# frozen_string_literal: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice tests.

@anakrish
Copy link
Collaborator

anakrish commented Apr 1, 2024

I have created an issue #191.

You could also update the bindings section of the main README.md

@thedavemarshall
Copy link
Contributor Author

There are a few other things I haven't gotten to yet besides adding the Ruby bindings tests to the github actions, but I think these could each be followup PRs.

  • custom Ruby exception class like Regorus::Error rather than using RuntimeError for more idiomatic error handling
  • Support calling eval_bool_query() , eval_allow_query() , eval_deny_query() from Ruby
  • Support calling add_extension() with Ruby code blocks - I have some ideas about this, but haven't had a chance to try anything out yet

@anakrish thank you so much for the feedback!

@anakrish
Copy link
Collaborator

anakrish commented Apr 2, 2024

@thedavemarshall I will go ahead and merge the PR. The remaining items including the github action could be done as separate PRs.

@anakrish
Copy link
Collaborator

anakrish commented Apr 2, 2024

Support calling add_extension() with Ruby code blocks - I have some ideas about this

Interesting!

@anakrish anakrish merged commit e86801b into microsoft:main Apr 2, 2024
4 checks passed
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.

2 participants