Merged
Conversation
Without this we'd get
```
$ ruby -Itest test/classlist/test_operation.rb
Run options: --seed 53158
..EE........E...
Finished in 0.010779s, 1391.5948 runs/s, 1298.8218 assertions/s.
1) Error:
TestClasslistOperation#test_all_the_operations:
NameError: uninitialized constant Classlist::Reset
test/classlist/test_operation.rb:97:in 'TestClasslistOperation#test_all_the_operations'
2) Error:
TestClasslistOperation#test_reset_as_manual_operations:
NameError: uninitialized constant Classlist::Reset
test/classlist/test_operation.rb:41:in 'TestClasslistOperation#test_reset_as_manual_operations'
3) Error:
TestClasslistOperation#test_reset:
NameError: uninitialized constant Classlist::Reset
test/classlist/test_operation.rb:35:in 'TestClasslistOperation#test_reset'
```
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where adding a plain Classlist instance to a chain of Classlist::Operations would incorrectly merge it into the last operation instead of treating it as an implicit Classlist::Add operation. The fix ensures consistent behavior whether you use Classlist.new("foo") or Classlist::Add.new("foo") when chaining operations.
Key changes:
- Added type conversion logic to treat plain
Classlistinstances asClasslist::Addoperations when used with the+operator - Added test coverage for the new behavior with both explicit
Classlist::Addand implicit plainClasslistadditions - Fixed missing require statement for
Classlist::Resetin the test file
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/classlist.rb | Added logic to convert plain Classlist instances to Classlist::Add operations before processing in the + operator |
| test/classlist/test_operation.rb | Added missing Reset require statement and two new tests verifying the fix works correctly |
| CHANGELOG.md | Documented the bug fix in the Fixed section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ie doing
Classlist.new("a b c") + Classlist.new("d e")
is the same as
Classlist.new("a b c") + Classlist::Add.new("d e")
and most notably doing
Classlist.new("a") + Classlist::Remove.new("a") + Classlist.new("b")
is the same as
Classlist.new("a") + Classlist::Remove.new("a") + Classlist::Add.new("b")
which it wasn't before.
Adding a "base" Classlist in that manner would merge it into the Remove
operation, effectively turning the statement into
Classlist.new("a") + Classlist::Remove.new("a b")
which is not the intended behavior
> lib/classlist.rb:12:3: Layout/EmptyLinesAfterModuleInclusion: Add an > empty line after module inclusion.
olepalm
approved these changes
Dec 8, 2025
This file contains hidden or 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
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.
Ie doing
is the same as
and most notably doing
is the same as
which it wasn't before.
Adding a "base" Classlist in that manner would merge it into the Remove
operation, effectively turning the statement into
which is not the intended behavior