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

Foldmethod auto set manual #49

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

KaminoU
Copy link

@KaminoU KaminoU commented Apr 20, 2023

• foldmethod auto set to manual
• avoid dup folding
• add/clean leading spaces before the fold marker comment string

KaminoU added 7 commits April 20, 2023 22:21
folding new options
• foldmethod auto set to manual
• add leading space to avoid ugly error message
• clear trailing spaces added by navbuddy when unfolding (i.e. other any trailing spaces stay in place)
• avoid duplicate folding
new folding options
code refactoring
@SmiteshP
Copy link
Owner

• foldmethod auto set to manual

I don't want to do this, foldmethod = manual is the default setting of neovim. And if foldmethod is set to something else, that means the user did that himself/herself. Navbuddy shouldn't just override it.

• add/clean leading spaces before the fold marker comment string

I also don't want to make changes to text while folding text. It might be surprise to the user to see spaces getting removed.

• avoid dup folding

This is ok 👍🏽

lua/nvim-navbuddy/init.lua Outdated Show resolved Hide resolved
Comment on lines 1 to 5
local USER_FOLDMETHOD = vim.o.foldmethod
local USER_FOLDMARKER = vim.o.foldmarker
local USER_FOLDMARKER_O, USER_FOLDMARKER_C = USER_FOLDMARKER:match("([^,]+),([^,]+)")
local CMD_FOLD_N_ZF = "normal! zf"
local CMD_FOLD_N_ZD = "normal! zd"
Copy link
Owner

Choose a reason for hiding this comment

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

No need of this? can be done inside the action function.

Copy link
Author

Choose a reason for hiding this comment

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

yes I know =}

As the command is used 2 times, if in the futur, it is changed, I only have to change one time...

Obviously our style is different. I dont like duplicated code, that's why...

local USER_FOLDMARKER_O, USER_FOLDMARKER_C = USER_FOLDMARKER:match("([^,]+),([^,]+)")
local CMD_FOLD_N_ZF = "normal! zf"
local CMD_FOLD_N_ZD = "normal! zd"
local api = vim.api
Copy link
Owner

Choose a reason for hiding this comment

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

Nope

Comment on lines 363 to 368
local nodeA_text =
api.nvim_buf_get_lines(for_buf, nodeA.scope["start"].line - 1, nodeA.scope["end"].line - 1 + 1, false)
local mid_text =
api.nvim_buf_get_lines(for_buf, nodeA.scope["end"].line - 1 + 1, nodeB.scope["start"].line - 1, false)
local nodeB_text =
api.nvim_buf_get_lines(for_buf, nodeB.scope["start"].line - 1, nodeB.scope["end"].line - 1 + 1, false)
Copy link
Owner

Choose a reason for hiding this comment

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

Don't make style changes as part of same pr please.

Copy link
Author

Choose a reason for hiding this comment

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

yep sry it's the auto format... ^^

@@ -64,50 +64,36 @@ local config = {
mappings = {
["<esc>"] = actions.close,
["q"] = actions.close,

Copy link
Owner

Choose a reason for hiding this comment

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

Same here, don't make style changes

@KaminoU
Copy link
Author

KaminoU commented Apr 22, 2023

• foldmethod auto set to manual

I don't want to do this, foldmethod = manual is the default setting of neovim. And if foldmethod is set to something else, that means the user did that himself/herself. Navbuddy shouldn't just override it.

I dont understand. The foldmethod is only an option to let nvim to know how to fold blocks.

In our case, I see Navbuddy as a tool that help us to quickly mark things to fold... And we only toggle this option to manual mode during a short time to write the fold marker comment string

That's what I do understand and that's why I do like Navbuddy, to help us to do quickly things, like the "which-key" plugin

Moreover, as we let the user to choose with options in navbuddy...

• add/clean leading spaces before the fold marker comment string

I also don't want to make changes to text while folding text. It might be surprise to the user to see spaces getting removed.

Would you mind to give a try please ? I do not break the code. On python for example, without leadings space, the linter raises ugly error/warnng messages. By writing this, it makes me realise that there is one case that I did not catch...

image

• avoid dup folding

This is ok 👍🏽

For the indentation/style sorry for this, it's the auto format when saving file ; no any intention to hurt your sensibility 😜

In any case thank you for your reply

@KaminoU
Copy link
Author

KaminoU commented Apr 22, 2023

I have modified my approach by simplifying the instructions to fit your requirements.

As said, to me, the main purpose of navbuddy is to accompany the users, and the capacity of marking dynamically fold comment string is a great feature.

I hope that this new version will better fit your initial idea of navbuddy.

I do not break any codes, I just give to the user the choice of doing thing dynamically if they want. And after the fold action, we restore the initial option

Cheers

@SmiteshP
Copy link
Owner

In our case, I see Navbuddy as a tool that help us to quickly mark things to fold... And we only toggle this option to manual mode during a short time to write the fold marker comment string

Oh ok. I was under the impression that changing fold method (even temporarily) would mess up folds created with different methods. But if it preserves other folds properly, then we have have this quick toggle as default anyways.

Would you mind to give a try please ? I do not break the code. On python for example, without leadings space, the linter raises ugly error/warning messages.

I am not against this feature, but I don't want this to be mixed with fold action. this could be its own action, something like format code or something action.

@KaminoU
Copy link
Author

KaminoU commented Apr 24, 2023

I am not against this feature, but I don't want this to be mixed with fold action. this could be its own action, something like format code or something action.

Hello,

I agree with you on this point. But... there is a lil but 😝

If we opt for the auto format stuff, it will format all the file... (that's what I did and you yelled on me earlier 😆)

In our case, we do things properly, we only change the fold marker comment string.

I invite you to give a try, with Packer it's easy, just add a branch and then you can delete it later, I believe that it will more relevant to see thing by yourself.

As said, I do not break anything ; I like flexibility and it gives a great opportunity to the user.

I have a personnal use case, I often read code and to know that I can fold/unfold quickly some portion of code is really a great feature. That's why I was glad to find Navbuddy

Cheers

P.S. I should rename reinit_foldmethod to restore_foldmethod (more relevant...)

local reinit_foldmethod = function()
	vim.o.foldmethod = USER_FOLDMETHOD
end

@SmiteshP
Copy link
Owner

If we opt for the auto format stuff, it will format all the file... (that's what I did and you yelled on me earlier 😆)

Thats not what I meant 😅

In our case, we do things properly, we only change the fold marker comment string.
I invite you to give a try, with Packer it's easy, just add a branch and then you can delete it later, I believe that it will more relevant to see thing by yourself.

I am not sure what you mean by "only changing the fold marker comment string". 🤔
Yep, I will try it out.

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