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

Move bundle_report rails compatibility logic into a class #137

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

Conversation

JuanVqz
Copy link
Member

@JuanVqz JuanVqz commented Feb 11, 2025

Description

I extracted it into its own class

Motivation and Context

I think it will be easier to understand the BundleReport.compatibility method and its usage
also I will need to use data from the incompatible_gems_by_state method in another code.

How Has This Been Tested?

There was an existing test case for the erb_output method, and I run it locally to make sure it still works.

Screenshots:

I will abide by the code of conduct

@JuanVqz JuanVqz requested review from etagwerker and arielj February 11, 2025 21:15
@JuanVqz JuanVqz force-pushed the move-rails-version-compatibility-into-its-own-class branch 2 times, most recently from 61699bf to 7f7a9df Compare February 11, 2025 22:05
Copy link

@arielj arielj left a comment

Choose a reason for hiding this comment

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

I added some comments

@JuanVqz JuanVqz force-pushed the move-rails-version-compatibility-into-its-own-class branch from b8e927b to 74e4bf6 Compare February 20, 2025 21:11
@JuanVqz JuanVqz force-pushed the move-rails-version-compatibility-into-its-own-class branch 2 times, most recently from bb757ad to 8f93530 Compare March 3, 2025 18:23
@JuanVqz JuanVqz requested a review from arielj March 3, 2025 18:24
@JuanVqz JuanVqz force-pushed the move-rails-version-compatibility-into-its-own-class branch 3 times, most recently from 04e5134 to e8c5911 Compare March 3, 2025 18:31
@JuanVqz
Copy link
Member Author

JuanVqz commented Mar 3, 2025

@arielj any chances you can review this, please?

@JuanVqz JuanVqz force-pushed the move-rails-version-compatibility-into-its-own-class branch 6 times, most recently from d7e4eb5 to aa6053d Compare March 3, 2025 20:54
I extracted it into a class because I think it will be easier to use the `incompatible_gems_by_state` data there.

Based on the code review
@JuanVqz JuanVqz force-pushed the move-rails-version-compatibility-into-its-own-class branch 3 times, most recently from 80a1716 to 078903a Compare March 3, 2025 21:34
Copy link

@arielj arielj left a comment

Choose a reason for hiding this comment

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

I only have that comment about the test that I think is not testing what it's meant to test, but the rest looks fine to me

@JuanVqz JuanVqz requested a review from arielj March 5, 2025 13:58
@JuanVqz
Copy link
Member Author

JuanVqz commented Mar 5, 2025

@arielj, test updated! could you please approve and merge?

@JuanVqz JuanVqz force-pushed the move-rails-version-compatibility-into-its-own-class branch from d1da583 to 550793d Compare March 5, 2025 14:02
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