Skip to content

fix(shared) better inner parameter selection #702

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

fix(shared) better inner parameter selection #702

wants to merge 10 commits into from

Conversation

przepompownia
Copy link
Contributor

The logic of including a point in a range needs -1 end_col offset
only if the point is from the cursor position
but not from the end of another range.

Fixes #700

@przepompownia
fix(shared) better inner parameter selection
c992e72 
The logic of including a point in a range needs `-1` end_col offset
only if the point is from the cursor position
but not from the end of another range.

Fixes nvim-treesitter#700
@przepompownia przepompownia marked this pull request as draft October 15, 2024 17:04
@przepompownia
Copy link
Contributor Author

przepompownia commented Oct 15, 2024 •
edited
Loading

Examples to cover by test:

vim.print(1, tonumber('1')) -- on `tonumber`
vim.print({1, '2'}, 3) -- on closing brace
vim.print({1, '2'}) -- on closing brace

Currently the change breaks function.outer selection (when the cursor is in the function body).

@przepompownia przepompownia marked this pull request as ready for review October 15, 2024 18:37
@przepompownia przepompownia marked this pull request as draft December 24, 2024 21:14
@przepompownia
Copy link
Contributor Author

Currently the change breaks function.outer selection (when the cursor is in the function body).

Fixed and added test cases.

@przepompownia przepompownia marked this pull request as ready for review December 24, 2024 22:55
kiyoon
kiyoon reviewed Dec 25, 2024
View reviewed changes
tests/select/python_spec.lua Outdated
run:compare_cmds("aligned_indent.py", { row = 1, col = 0, cmds = { "d]a", "v]ad", "c]a" } })
run:compare_cmds("selection_mode.py", { row = 2, col = 4, cmds = { "dam", "dVam", "vamd", "Vamd" } })
run:compare_cmds("selection_mode.py", { row = 2, col = 4, cmds = { "dam", "dVam", "vamd", "Vamd", "dG" } })
run:compare_cmds("selection_mode.py", { row = 3, col = 8, cmds = { "kdG", "dam", "dVam", "vamd", "Vamd" } })
Copy link
Collaborator

Choose a reason for hiding this comment

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

dG, kdG motions break when the reference code changes slightly. Maybe better if you do something like d]M which is more meaningful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention was comparing the plugin action with something with something independent, but still reliable. IIRC the test compares results and expect only equation or only difference. In the former case I got passed test when all keystrokes gave the same bad result. While the test is defined as now, let allow to compare with something reliable (line 25 alraedy contains d;).

@kiyoon
Copy link
Collaborator

kiyoon commented Dec 25, 2024

I don't use neovim nightly so I could only find time now to test it..

Thanks for reporting it. It's definitely odd and in master branch, there is no such issue.

I'd like to accept this PR but I didn't follow much on main and the function you modified doesn't exist in master..

Can you investigate a little why this is an issue on main and not master? Thank you!

kiyoon
kiyoon reviewed Dec 25, 2024
View reviewed changes
lua/nvim-treesitter-textobjects/shared.lua
---@param end_col_offset integer?
---@return boolean
local function is_in_range(range, row, col)
local function is_in_range(range, row, col, end_col_offset)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a brief comment on why the end_col_offset is needed (in what situation,) and a link to the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a brief comment on why the end_col_offset is needed (in what situation,) and a link to the issue.

@przepompownia , can you address the requested comments by @kiyoon ?

@PMassicotte
Copy link
Contributor

I noticed this PR hasn't progressed in a while. Would you be open to me helping complete it by addressing the requested changes?

@przepompownia
Copy link
Contributor Author

I noticed this PR hasn't progressed in a while. Would you be open to me helping complete it by addressing the requested changes?

Please feel free to convert this PR to a more convincing version.

@PMassicotte
Copy link
Contributor

I have tested it on my side for a couple of days and have not faced any issues with any *.inner selection modes. I am not too sure on how to add changes to your PR. I will take a look.

@przepompownia
Copy link
Contributor Author 

 $ make docs
NVIM_TS=.test-deps/nvim-treesitter .test-deps/nvim-linux-x86_64/nvim-linux-x86_64/bin/nvim -l scripts/update-readme.lua
E5113: Lua chunk: ...x-x86_64/share/nvim/runtime/lua/vim/treesitter/query.lua:372: No parser for language "apex"
stack traceback:
        [C]: in function 'assert'
        ...x-x86_64/share/nvim/runtime/lua/vim/treesitter/query.lua:372: in function 'fn'
        ...inux-x86_64/share/nvim/runtime/lua/vim/func/_memoize.lua:78: in function 'fn'
        ...inux-x86_64/share/nvim/runtime/lua/vim/func/_memoize.lua:78: in function 'get'
        ./lua/nvim-treesitter-textobjects/shared.lua:424: in function 'fn'
        ./lua/nvim-treesitter-textobjects/shared.lua:42: in function 'available_textobjects'
        scripts/update-readme.lua:38: in main chunk
make: *** [Makefile:124: docs] Error 1

so at the moment I cannot regenerate docs on my local side.

@PMassicotte
Copy link
Contributor

Looks like the apex parser is missing.

 $ make docs
NVIM_TS=.test-deps/nvim-treesitter .test-deps/nvim-linux-x86_64/nvim-linux-x86_64/bin/nvim -l scripts/update-readme.lua
E5113: Lua chunk: ...x-x86_64/share/nvim/runtime/lua/vim/treesitter/query.lua:372: No parser for language "apex"
stack traceback:
        [C]: in function 'assert'
        ...x-x86_64/share/nvim/runtime/lua/vim/treesitter/query.lua:372: in function 'fn'
        ...inux-x86_64/share/nvim/runtime/lua/vim/func/_memoize.lua:78: in function 'fn'
        ...inux-x86_64/share/nvim/runtime/lua/vim/func/_memoize.lua:78: in function 'get'
        ./lua/nvim-treesitter-textobjects/shared.lua:424: in function 'fn'
        ./lua/nvim-treesitter-textobjects/shared.lua:42: in function 'available_textobjects'
        scripts/update-readme.lua:38: in main chunk
make: *** [Makefile:124: docs] Error 1

so at the moment I cannot regenerate docs on my local side.

I think this is because you need to have all parsers installed, which must be the case on CI.

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

Reviewers

@kiyoon kiyoon kiyoon left review comments

+1 more reviewer

@PMassicotte PMassicotte PMassicotte left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@przepompownia @kiyoon @PMassicotte