-
-
Notifications
You must be signed in to change notification settings - Fork 912
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
fix: Run actiontext migrations for rails >= 6 #1365
fix: Run actiontext migrations for rails >= 6 #1365
Conversation
6e8602b
to
1f9996e
Compare
1f9996e
to
c179dcf
Compare
@@ -26,7 +26,7 @@ def create | |||
def load | |||
load_environment | |||
|
|||
if rails_version > 5 && bundle.includes?('actiontext') | |||
if rails_version >= 6 && bundle.includes?('actiontext') | |||
add_action_text_migration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it can be used in versions below if included in the Gemfile that is why the other condition. (bundle.includes?('actiontext'))
For example: https://www.driftingruby.com/episodes/using-action-text-in-a-rails-5-2-application
I notice now that I forgot to test the actiontext on other versions. It's only tested on Rails 6. If we decide to not test on 5.2, I see no problem with this adjustment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice now that I forgot to test the actiontext on other versions. It's only tested on Rails 6. If we decide to not test on 5.2, I see no problem with this adjustment.
Unless we use a patched fork to work around, rails/actiontext
gem dependencies for rails < 6
can not be met from official sources.
We can also use bundle.includes?('actiontext')
only rather than checking the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right. We can ignore the version. I don't see any problem with this change, but I think we can do even better. Let me take another look at the project.
EDIT 1: LGTM!
EDIT2: I was looking at this file and wondering why I didn't do something similar, but I don't think something like that is necessary for this case. We can do this refactoring later if the need arises. Thank you for pointing this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, maybe later it becomes a refactoring candidate as we do not rely on version but rather on definition of actiontext
.
Is this build failing, is that what caused this change? I guess I'm wondering what this affects and how you knew to change this. |
Yes, I noticed it because of the build failure while fixing Though I think I will have to add shoulda-matchers/lib/shoulda/matchers/active_record/have_rich_text_matcher.rb Lines 69 to 71 in 402abf8
|
- as actiontext is made available on rails 6.0
c179dcf
to
8ac98be
Compare
I'm sorry, but I'm not sure how this fix will help you with the CI breaking on #1350. The problem there is ActionText::RichText being called without being defined. For that reason, we compared with a string as you can see in this comment: #1263 (comment) There are tests when the ActionText is not defined: https://github.com/thoughtbot/shoulda-matchers/blob/master/spec/unit/shoulda/matchers/active_record/have_rich_text_matcher_spec.rb#L51 We need to find another way to check if the attribute is an ActionText::RichText instance without using the class. EDIT: I'm almost sure that this change will fix the problem on #1350: defined?(ActionText::RichText) && @subject.send(rich_text_attribute).instance_of?(ActionText::RichText) PS: But I think it should be done on #1350. |
While running tests of this repo, defined?(ActionText::RichText) && @subject.send(rich_text_attribute).instance_of?(ActionText::RichText) Yes, I would have to do that at #1350 . I didn't wanted |
Ah, I see. The build isn't broken, this seems to be more of a consistency thing, as we don't check for a Rails version in the tests either: shoulda-matchers/spec/unit/shoulda/matchers/active_record/have_rich_text_matcher_spec.rb Line 5 in 59f7c0b
|
... as actiontext is made available on rails 6.0