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.

Sign up for GitHub

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

Jump to bottom

Add support for ruby for polyglot piranha #649

Merged
merged 1 commit into from May 18, 2024
Merged

Add support for ruby for polyglot piranha #649

merged 1 commit into from May 18, 2024

Conversation

Bennet-Sunder
Copy link
Contributor

@Bennet-Sunder Bennet-Sunder commented Mar 1, 2024 •
edited

This PR adds support to clean up ruby code in polyglot piranha.

ketkarameya
ketkarameya reviewed Mar 1, 2024
View reviewed changes
Copy link
Collaborator

@ketkarameya ketkarameya left a comment

Choose a reason for hiding this comment

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

Can we have a description and test cases for this PR?

src/cleanup_rules/ruby/rules.toml Outdated Show resolved Hide resolved
src/cleanup_rules/ruby/rules.toml Outdated Show resolved Hide resolved
src/models/default_configs.rs Outdated Show resolved Hide resolved
@ketkarameya
Copy link
Collaborator

@dvmarcilio please let me know if adding this support is alright, and ya'll are ready to maintain it as FF cleanup track.

@przemyslawbialon
Copy link

Hi, what's the status of this PR? is there any chance it will be merged? What is the expected timeline? I'd love to run a pov in my organization but we need support for ruby.

@ketkarameya
Copy link
Collaborator

ketkarameya commented Mar 19, 2024 •
edited

Hey! Thank you for reaching out. I am not leading this change, so I have no idea when ruby support will be there.
Currently, as a repo we do not have any priority to add ruby support, because uber is not a big ruby house. But we are open for contributions. Actually, Idk if folks at Uber want this, and want to prioritize this. @dvmarcilio thoughts? Is this on your agenda? If not, we can still review and land this diff whenever it is ready?

@Bennet-Sunder
Copy link
Contributor Author

@przemyslawbialon This PR is almost done and will be ready for review in a week or two. In the meanwhile if you wish to evaluate it, you will be able to get that done by cloning this branch and trying it out locally.

@dvmarcilio
Copy link
Collaborator

dvmarcilio commented Mar 21, 2024 •
edited

Sorry for the delay. As @ketkarameya said, this is not our priority. However, community PRs are always welcome and we appreciate @Bennet-Sunder 's work.
We're happy to review it given this check all the boxes, including adding comprehensive tests, just as the rules for the other languages.

@Bennet-Sunder Bennet-Sunder force-pushed the ruby_support branch 2 times, most recently from d12404d to b03a051 Compare April 18, 2024 09:32
@Bennet-Sunder Bennet-Sunder marked this pull request as ready for review April 18, 2024 09:32
@Bennet-Sunder Bennet-Sunder changed the title [WIP]: add support for ruby Add support for ruby for polyglot piranha Apr 18, 2024
@Bennet-Sunder Bennet-Sunder force-pushed the ruby_support branch 2 times, most recently from d7cd516 to adf5c7e Compare April 18, 2024 09:48
@Bennet-Sunder
Copy link
Contributor Author

@ketkarameya @dvmarcilio I have added the tests for the changes, could you pls review the PR when you have bandwidth.

@Bennet-Sunder Bennet-Sunder force-pushed the ruby_support branch 2 times, most recently from 3cd59d8 to 275bf0b Compare May 3, 2024 03:57
@ketkarameya
Copy link
Collaborator

@donald-pinckney please shepherd this PR

@donald-pinckney
Copy link
Contributor

Hi @Bennet-Sunder, all of this looks like amazing work for Ruby! Let's get this merged in, and any additional useful cleanups can always be added later :)

@ketkarameya
Copy link
Collaborator

Thanks for your contributions @Bennet-Sunder

@ketkarameya ketkarameya merged commit 7c05ede into uber:master May 18, 2024
10 checks passed
@Bennet-Sunder
Copy link
Contributor Author

Thanks @donald-pinckney @ketkarameya for your time in reviewing and merging this PR :)

@ketkarameya
Copy link
Collaborator

I merged it but it failed on main. I think the reason is something similar to a discussion u had started. Could tell me how u got about it?

@donald-pinckney donald-pinckney mentioned this pull request May 20, 2024
Merged
@donald-pinckney
Copy link
Contributor

PR #663 should fix the broken build introduced by this PR.

@Bennet-Sunder
Copy link
Contributor Author

@przemyslawbialon Support for ruby is now merged and should be available in the next release.

@Bennet-Sunder Bennet-Sunder mentioned this pull request May 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donald-pinckney donald-pinckney donald-pinckney approved these changes

@ketkarameya ketkarameya ketkarameya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Bennet-Sunder @ketkarameya @przemyslawbialon @dvmarcilio @donald-pinckney