Skip to content

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 arielj and etagwerker 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
@JuanVqz
Copy link
Member Author

JuanVqz commented Mar 6, 2025

Since the requested change by @arielj was only for the test output. (if this hasn't been approved), I will merge the PR at the end of the day to unblock another work.

@JuanVqz JuanVqz merged commit 13181a7 into main Mar 7, 2025
8 checks passed
@JuanVqz JuanVqz deleted the move-rails-version-compatibility-into-its-own-class branch March 7, 2025 14:57
@JuanVqz JuanVqz mentioned this pull request Mar 7, 2025
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