Skip to content

Update to activerecord 5#533

Merged
mockdeep merged 1 commit into
stringer-rss:masterfrom
dperson:update-activerecord
Feb 23, 2021
Merged

Update to activerecord 5#533
mockdeep merged 1 commit into
stringer-rss:masterfrom
dperson:update-activerecord

Conversation

@dperson
Copy link
Copy Markdown
Contributor

@dperson dperson commented Feb 22, 2021

Followed the instructions at https://www.fastruby.io/blog/rails/upgrades/upgrade-rails-from-4-2-to-5-0.html to migrate to the newer version. Then fixed the pec/support/active_record.rb calls to work with the new API.

@mockdeep
Copy link
Copy Markdown
Collaborator

mockdeep commented Feb 22, 2021

@dperson Thanks for jumping into this!

One thing that concerns me about the upgrade is belongs_to being required by default. In my short time working on Stringer, it seems like things aren't very tightly constrained, so I could see that throwing an error on any missing data. We might want to set that to false for the time being:

config.active_record.belongs_to_required_by_default = false

@dperson
Copy link
Copy Markdown
Contributor Author

dperson commented Feb 22, 2021

@mockdeep it has been running well for me, but I'll be glad to change the configuration to maintain the historical behavior.

@dperson dperson force-pushed the update-activerecord branch 2 times, most recently from 49d43d2 to f77b0f7 Compare February 22, 2021 21:49
@mockdeep
Copy link
Copy Markdown
Collaborator

mockdeep commented Feb 22, 2021

@dperson actually, it looks like the configuration defaults to false outside of Rails apps. If it was enabled, I'm pretty sure we'd have failing tests. New feeds by default don't have a group, so it wouldn't allow you to add a new feed if it was required. You can remove the config/application.rb you added. (I don't think it's doing anything anyway, since the file doesn't seem to be getting loaded anywhere.)

Sorry for the back and forth!

@dperson dperson force-pushed the update-activerecord branch from f77b0f7 to b60dc28 Compare February 23, 2021 00:11
@dperson
Copy link
Copy Markdown
Contributor Author

dperson commented Feb 23, 2021

@mockdeep no problem. I deleted the commit that added the file, and pushed it back to the branch. I'm just glad that there are fixes / updates coming once again. Thank you!

@mockdeep
Copy link
Copy Markdown
Collaborator

mockdeep commented Feb 23, 2021

I'm just glad that there are fixes / updates coming once again.

Me too. Thanks!

@mockdeep mockdeep merged commit 933fd07 into stringer-rss:master Feb 23, 2021
@dperson dperson deleted the update-activerecord branch February 23, 2021 03:19
mockdeep added a commit that referenced this pull request Feb 28, 2021
@mockdeep
Deps: fix delayed job dependency
12fa9b3 
I'm a little surprised this was possible, but #533 introduced a version
of ActiveRecord that did not match the constraints of
`delayed_job_active_record`. I thought bundler was more strict, but it
somehow allowed this one to slip through. This fixes the issue.
This was referenced Feb 28, 2021
Merged
Merged
mockdeep added a commit that referenced this pull request Feb 28, 2021
@mockdeep
Deps: fix delayed job dependency (#546)
22d5233 
I'm a little surprised this was possible, but #533 introduced a version
of ActiveRecord that did not match the constraints of
`delayed_job_active_record`. I thought bundler was more strict, but it
somehow allowed this one to slip through. This fixes the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dperson @mockdeep