Skip to content

Added a test for the control+left and control+right keyboard shortcuts#503

Open
cosmincalinov wants to merge 2 commits intoilai-deutel:masterfrom
cosmincalinov:add-tests
Open

Added a test for the control+left and control+right keyboard shortcuts#503
cosmincalinov wants to merge 2 commits intoilai-deutel:masterfrom
cosmincalinov:add-tests

Conversation

@cosmincalinov
Copy link

Checked if the Ctrl + and Ctrl + keyboard shortcuts behaved correctly in the editor.

Copy link
Owner

@ilai-deutel ilai-deutel left a comment

Choose a reason for hiding this comment

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

Thanks a lot for adding tests, increasing coverage is always great!

There are actually already tests for previous/next word shortcuts, see editor_move_cursor_left and editor_move_cursor_right. However I like that your PR tests using editor.process_keypress(...), which is a higher-level function, so it's closer to the "public" interface, and more useful to test.

Instead of creating a new test, I think it could make sense to update the existing ones to use editor.process_keypress(...) instead of editor.move_cursor(...), what do you think?

@ilai-deutel
Copy link
Owner

@all-contributors Please add @cosmincalinov for test.

@allcontributors
Copy link
Contributor

@ilai-deutel

I've put up a pull request to add @cosmincalinov! 🎉

@cosmincalinov
Copy link
Author

Thanks a lot for adding tests, increasing coverage is always great!

There are actually already tests for previous/next word shortcuts, see editor_move_cursor_left and editor_move_cursor_right. However I like that your PR tests using editor.process_keypress(...), which is a higher-level function, so it's closer to the "public" interface, and more useful to test.

Instead of creating a new test, I think it could make sense to update the existing ones to use editor.process_keypress(...) instead of editor.move_cursor(...), what do you think?

Agreed, it would be way easier to read and more intuitive i think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants