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

Support newer rails and test against it #164

Merged
merged 3 commits into from
Mar 2, 2023
Merged

Conversation

maths22
Copy link
Contributor

@maths22 maths22 commented Mar 1, 2023

No description provided.

@dangerpr-bot
Copy link

dangerpr-bot commented Mar 2, 2023

1 Warning
⚠️ There're library changes, but not tests. That's OK as long as you're refactoring existing code.

Generated by 🚫 Danger

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Thanks. Let's document compatibility in README, add a CHANGELOG entry and see my comment on CI matrix please.

exclude:
- ruby: '2.6.2'
active_record: '~> 7.0.0'
name: test (ruby=${{ matrix.ruby }}, postgresql=${{ matrix.postgresql }}, active_record=${{ matrix.active_record }})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer it if we kept the original entry matrix format to be consistent with other .yml, and not use exclude because this is 2 x 2 x 3 and will grow out of control fast every time we add a new ruby version or a new AR version or a new PGsql version. The list of excludes becomes hard to manage. Add the active_record values that should have been there in the first place?

@@ -84,7 +84,7 @@ class TeamsEndpoint < Grape::API
if team
team.ping_if_active!

team.update_attributes!(
team.update!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only substantive change of this PR. update_attribute! has been an alias to update! since rails 4.0 and was finally deprecated in rails 6.1 and dropped in rails 7.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it!

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Nit on the CHANGELOG.

Let's increment the version to 2.1.0 in this PR (version.rb and CHANGELOG) since this is a new feature and not a patch.

CHANGELOG.md Outdated
@@ -3,6 +3,7 @@
### 2.0.2 (Next)

* [#163](https://github.com/slack-ruby/slack-ruby-bot-server/pull/163): Updated releasing documentation - [@crazyoptimist](https://github.com/crazyoptimist).
* [#164](https://github.com/slack-ruby/slack-ruby-bot-server/pull/164): Support newer rails and test against it - [@maths22](https://github.com/maths22).
Copy link
Collaborator

@dblock dblock Mar 2, 2023

Choose a reason for hiding this comment

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

Let's write which version of Rails, e.g. "Add support for Rails 7", it won't be "newer" forever.

@@ -84,7 +84,7 @@ class TeamsEndpoint < Grape::API
if team
team.ping_if_active!

team.update_attributes!(
team.update!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it!

@dblock
Copy link
Collaborator

dblock commented Mar 2, 2023

Thanks, merging this. Will do a release soon. There might be other libraries that need similar updates, https://github.com/slack-ruby/slack-ruby-bot-server-events or https://github.com/slack-ruby/slack-ruby-bot-server-rtm, appreciate any help you can give!

@dblock dblock merged commit a04145b into slack-ruby:master Mar 2, 2023
@maths22
Copy link
Contributor Author

maths22 commented Mar 2, 2023

I use slack-ruby-bot-server-events in the same project where I use this library and it didn't require any changes; skimming over slack-ruby-bot-server-rtm it doesn't appear to directly interact with any of the activerecord-layer stuff from this repo either.

@maths22 maths22 deleted the rails70 branch March 2, 2023 16:05
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.

3 participants