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

Refactor key actor test #645

Merged
merged 3 commits into from
Mar 24, 2024
Merged

Conversation

tompng
Copy link
Member

@tompng tompng commented Mar 19, 2024

Description

test_key_actor_*.rb has these kind of ideoms

# assert line to be `ab█cd` (█ is cursor position)
assert_line('abcd')
assert_byte_pointer_size('ab')
# It was asserting internal state to be consistent with `ab█cd`
# We don't have these internal state anymore. Having them as a state was a design failure, IMO.
assert_cursor(2)
assert_cursor_max(4)

We only need this assertion.

# assert line to be `ab█cd` (█ is cursor position)
assert_line_around_cursor('ab', 'cd')

Commits

The "Autofix" commit is done by this script https://gist.github.com/tompng/215d11c9cf2c825755960b9b934a7167
Then, manually fixed remaining things by hand.

def assert_line(expected)
expected = convert_str(expected)
assert_equal(expected, @line_editor.line)
def assert_cursor_line(before, after)
Copy link
Member

Choose a reason for hiding this comment

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

I think assert_cursor_line is not clear regarding what it asserts. I think it would be better for this method name to include the meaning of before the cursor and after the cursor.
How about renaming it to assert_line_around_cursor?

Copy link
Member Author

Choose a reason for hiding this comment

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

The name assert_line_around_cursor sounds good. Thanks, renamed.

@tompng tompng force-pushed the refactor_key_actor_test branch from cf16222 to 6376184 Compare March 24, 2024 09:17
Copy link
Member

@ima1zumi ima1zumi left a comment

Choose a reason for hiding this comment

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

LGTM 🌟

@ima1zumi ima1zumi merged commit e477380 into ruby:master Mar 24, 2024
40 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Mar 24, 2024
(ruby/reline#645)

* Add assertion assert_cursor_line to test helper

* Autofix key_actor test to use assert_cursor_line

* Rename the assertion to assert_line_around_cursor and remove other assertions for line and cursor

ruby/reline@e4773800c6
@tompng tompng deleted the refactor_key_actor_test branch March 24, 2024 12:35
artur-intech pushed a commit to artur-intech/ruby that referenced this pull request Apr 26, 2024
(ruby/reline#645)

* Add assertion assert_cursor_line to test helper

* Autofix key_actor test to use assert_cursor_line

* Rename the assertion to assert_line_around_cursor and remove other assertions for line and cursor

ruby/reline@e4773800c6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants