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

Add arbitrary lock on class level too #499

Merged

Conversation

pandwoter
Copy link
Contributor

@pandwoter pandwoter commented Jan 28, 2022

@pandwoter pandwoter marked this pull request as ready for review January 28, 2022 13:11
Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

@pandwoter This is a great start.

I noticed that you created a new method called .with_advisory_record_lock. I'd like the key: parameter added to the existing .with_advisory_lock class method. e.g. "it can be passed a column or a key" It's going to arguably overload it, but I don't want to introduce another method.

And I'm glad that you added additional .advisory_record_lock and advisory_record_unlock methods. I'd like those all to be renamed .advisory_lock so that they are symmetrical with the instance methods.

Also, I think an additional refactor, once the class methods are dialed in, is to have the instance methods delegate to the class methods where possible.

Thank you!

@pandwoter
Copy link
Contributor Author

I noticed that you created a new method called .with_advisory_record_lock. I'd like the key: parameter added to the existing .with_advisory_lock class method. e.g. "it can be passed a column or a key" It's going to arguably overload it, but I don't want to introduce another method.

Done! ✅

And I'm glad that you added additional .advisory_record_lock and advisory_record_unlock methods. I'd like those all to be renamed .advisory_lock so that they are symmetrical with the instance methods.

Yeah, I named this method like so due that we already have advisory_lock scope and we can't define a method with this exact name 😢 (

scope :advisory_lock, (lambda do |column: _advisory_lockable_column, function: advisory_lockable_function|
)

Also, I think an additional refactor, once the class methods are dialed in, is to have the instance methods delegate to the class methods where possible.

Done ✅

Any thoughts regarding these scopes @bensheldon? To have exact same interface on a class and instance level we need to do something with it...

@bensheldon bensheldon added the refactor Code changes that do not introduce new features label Jan 31, 2022
@bensheldon
Copy link
Owner

bensheldon commented Jan 31, 2022

Any thoughts regarding these scopes @bensheldon? To have exact same interface on a class and instance level we need to do something with it...

I think what we have right now is mergable; it accomplishes what I set out. You do surface an issue of there are several methods to overload. I'm thinking:

  • create separate class-level *_key and *_column methods e.g. .advisory_lock_key/.advisory_lock_column, and advisory_unlock_key/advisory_unlock_column
  • And then create the combined class methods that can take either a column or a key and delegate to the more specific method.

@bensheldon bensheldon merged commit 14fb56f into bensheldon:main Feb 5, 2022
@bensheldon
Copy link
Owner

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code changes that do not introduce new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants