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

Handle ActiveRecord::Locking::LockingType #2014

Merged

Conversation

kbruccoleri
Copy link
Contributor

@kbruccoleri kbruccoleri commented Sep 6, 2024

Motivation

ActiveRecord::Locking::LockingType is used for the lock_version column to handle Optimistic Locking:

https://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html

This type was falling into the else block and causing a Sorbet Error:

Expected type ::Object, got type ActiveRecord::Locking::LockingType with value #<ActiveModel::Type::Intege...ange=-2147483648...2147483648> (SorbetError)

Experiencing an issue after upgrading to latest Tapioca for Rails 7.2 compatibility.

Implementation

The column_type is really just a wrapper for a potentially nilable integer, so we make a specific exception for it.

Tried to follow the convention around it with regards to determining if particular column is nilable or not.

Tests

No existing test file for lib/tapioca/dsl/helpers/active_record_column_type_helper.rb

Shoutout to @cquinones100 for co-authoring

@kbruccoleri kbruccoleri requested a review from a team as a code owner September 6, 2024 20:36
@kbruccoleri
Copy link
Contributor Author

I have signed the CLA!

@amomchilov amomchilov self-assigned this Sep 12, 2024
@amomchilov amomchilov added the enhancement New feature or request label Sep 12, 2024
Copy link
Contributor

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

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

Hey Kevin, thanks for your first contribution!

Can you please credit @cquinones100 correctly by adding this to the end of your commit description:

Co-authored-by: NAME <[email protected]>

That'll make GitHub properly show you both as authors

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

@amomchilov
Copy link
Contributor

There's some unrelated CI failures btw. I'm working through those now.

@kbruccoleri kbruccoleri force-pushed the handle-activerecord-locking-lockingtype branch from 507f1ab to 2d5e3c4 Compare September 12, 2024 19:02
@kbruccoleri
Copy link
Contributor Author

@amomchilov Thanks for the feedback and the guidance, I have accepted all of your edits and updated the co-authors.

@amomchilov
Copy link
Contributor

amomchilov commented Sep 12, 2024

@kbruccoleri Excellent! I'll follow up on this once I've sorted out the CI issues

In the meantime, you please fix the lint failure? My hand-scribbled indentation suggestions were incorrect haha.

bundle exec rubocop --auto-correct lib/tapioca/dsl/helpers/active_record_column_type_helper.rb

@egiurleo
Copy link
Contributor

Hi @kbruccoleri! You can rebase on main, which should fix the CI issues. If you could also fix the linting issues, that would be much appreciated!

@kbruccoleri kbruccoleri force-pushed the handle-activerecord-locking-lockingtype branch from 436a670 to 46688aa Compare September 26, 2024 14:07
@kbruccoleri
Copy link
Contributor Author

Hi @kbruccoleri! You can rebase on main, which should fix the CI issues. If you could also fix the linting issues, that would be much appreciated!

@egiurleo Thanks for flagging - I believe I am rebased on main now and have bin/style passing.

@kbruccoleri
Copy link
Contributor Author

Hi there! Is there anything I can do to get this over the finish line? It seems like some checks might be failing for other reasons.

@amomchilov amomchilov changed the title Handle ActiveRecord::Locking::LockingType Handle ActiveRecord::Locking::LockingType Dec 11, 2024
ActiveRecord::Locking::LockingType is used for the lock_version column to handle Optimistic Locking:

https://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html

- This type was falling into the else block and causing a Sorbet Error:

Expected type ::Object, got type ActiveRecord::Locking::LockingType with value #<ActiveModel::Type::Intege...ange=-2147483648...2147483648> (SorbetError)

The column_type is really just a wrapper for a potentially nilable integer, so we make a specific exception for it.

Co-authored-by: Carlos Quinones <[email protected]>
Co-authored-by: Alexander Momchilov <[email protected]>
@amomchilov amomchilov force-pushed the handle-activerecord-locking-lockingtype branch from 46688aa to 4f94436 Compare December 11, 2024 22:48
@amomchilov
Copy link
Contributor

@kbruccoleri I don't know what was up with those CI failures. I rebased, fixed the merge conflict, and started another CI run. I'll have a look at its result in the morning.

@amomchilov amomchilov merged commit 3bf4e21 into Shopify:main Dec 12, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants