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

Add ruleset.rules accessor #3

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

Conversation

reid-rigo
Copy link

Hey @hmarr, really appreciate your work on this. I'm working on some Ruby bindings to this library, and this little tweak would be helpful to provide an accessor to the rules. I'm not experienced with Rust, so please advise if you think this should be done a different way, like via a getter.

@hmarr
Copy link
Owner

hmarr commented Mar 8, 2025

Thanks! You're right: getter would be preferable here. The constructor uses the rules vec to build the Matcher, and this change would mean that if you have a mutable reference to a RuleSet you'd be able to mutate the rules without rebuilding the Matcher.

Something like this ought to do it. This returns a reference to a slice of the rules rather than the mutable vector.

    pub fn rules(&self) -> &[Rule] {
        &self.rules
    }

I actually wrote Ruby bindings for this library a while back. It's in a private project owned by my ex-employer so I don't have access, but let me know if you need a hand.

@reid-rigo reid-rigo changed the title Make ruleset.rules public Add ruleset.rules accessor Mar 10, 2025
@reid-rigo
Copy link
Author

All right, I've updated the PR to use a function. Thanks for the feedback and feel free to make edits to your liking.

Awesome to hear that you've built Ruby bindings to this. We have an internal codeowners library built in Ruby, but it's too slow to use for some live purposes. I'm using magnus to make a gem and I'd love to get your feedback when I find the time to get things working.

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