Add support for Ruby 3 keyword arguments + add method to remove adapter #51
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Being able to check for and remove adapters is useful since adding an adapter that is already registered leads to an error. Removing an adapter will restore the old behavior of the target again.
Ruby 3 no longer captures keyword arguments in `*args`, which meant that keyword arguments got removed when the method was isolated. This change works the same in Ruby 2.6, 2.7, and 3.0. This should not be a breaking change, since all `details_message` procs that only accept two arguments will get the keyword arguments added as a hash to the end of the `args` parameter. If you implement a callback that receives three arguments, then kwargs will be emitted as the third argument instead.
|
Heh, better late than never. 😅 I also rebased on latest |
|
Thanks! |
|
Is this an issue, or is it intended? #70 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of this pull request?
This fixes two bugs I've been having in my project using this.
Removing adapters
The first bug is that code reloading in Rails causes double registration of adapters. Code reloading could be disabled by including the class earlier and/or making the adapter only register once at startup, but then things fail in development while making changes to the class.
My current workaround was to manually access the
Isolator.adaptersarray and removing the entry before callingIsolator.isolate.Ruby 3 keyword arguments
The second bug came up when upgrading to Ruby 3, as one of my classes that are isolated uses keyword arguments. In Ruby 3 a method like
def x(*args)will no longer accept keyword arguments, and the modded method uses a construct like that to catch arguments.What changes did you make?
I added the following methods:
Isolator.has_adapter?- to check if an adapter is there or not. Uses same key normalization logic asisolate.Isolator.remove_adapter- to remove an adapter and restore the modded class.I changed some tests:
I changed some behavior:
details_messagecallback does not require arguments to be accepted anymore, taking only(object).details_messageaccepts three arguments, then it gets(object, args, kwargs).details_messageaccepts exactly 2 arguments, it will get(object, args + [kwargs])to be backwards compatible.Isolator.add_patch_methodand added a similar private class method instead. I don't think it was intended to be public.Is there anything you'd like reviewers to focus on?
Restoring logic
Restoring the mod is very difficult since you cannot remove an included/prepended module in Ruby. The closest I could do was to remove the method on the dynamically-generated module so it becomes an empty method that doesn't do anything.
That means that there's no clean restoration, and by checking the ancestor chain you could tell a class was isolated before. If you keep adding and removing isolation on a class it will keep getting more and more empty modules in its inheritance chain.
I regarded this as a small problem, and I suspect that the most common case is for code reloading in development, at which point the classes will also be recreated each time and having heir ancestor chains cleared implicitly.
Please verify this assumption.
Testing structure
The tests I added follow RSpec/BDD style, which most other tests do not. I was very unsure about the style the tests are supposed to be written in. Please let me know if you want them structured differently.
Placement of
has_adapter?andremove_adapterI placed
remove_adapterclose toisolatesince they are a pair, buthas_adapter?next to theadapterdefinition. I'm not sure whyisolateis defined in its own module, so I might be placing things in the wrong place.Is this a good place to have the new methods?
Removal of
Isolator.add_patch_methodI don't think this was intended to be a public method. Was it a mistake to remove it?
Bigger PR
Should I split this PR into two smaller ones instead? I joined this up because it followed naturally for me (in order to fix the kwargs issue, I needed tests, and in order to make isolated tests I needed to be able to restore things, etc.).
I'll use my own fork to get my project moving again for now, but if you want two different PRs I will close this and create new branches with more focused changes and open them in series instead.
Checklist