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

Add :write-without-auto-format commands #4909

Closed

Conversation

zummenix
Copy link
Contributor

This adds two commands to write and force write without auto format.

Closes #4853

Copy link
Member

@dead10ck dead10ck left a comment

Choose a reason for hiding this comment

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

Implementation wise this looks fine, though I think the command names are very long. I also wonder if it wouldn't make sense to make this an argument of the command using something like a CLI switch. I think kakoune does this if I'm not mistaken. What do you think @archseer @the-mikedavis?

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 27, 2022
@the-mikedavis
Copy link
Member

Yeah I like kakoune's flags for this. It could be done as :write --no-fmt. That would mean we couldn't write a file named --no-fmt but we could also introduce -- to separate flags from literal arguments. (:write --no-fmt -- --no-fmt writes a file --no-fmt without formatting.)

@zummenix
Copy link
Contributor Author

Personally, I don't have a problem with the length of the commands, because the prompt autocomplete works really nice.

Are there precedents of commands that accept args/flags as proposed (I don't see any)?
What other commands that could benefit from supporting args/flags in them?
How args/flags should be displayed in prompt autocomplete (UX/UI)?
How args/flags should be documented (discoverability story)?

@the-mikedavis
Copy link
Member

Are there precedents of commands that accept args/flags as proposed (I don't see any)?

Not yet; this would be a first. But other commands could benefit from it (existing commands and future commands probably)

What other commands that could benefit from supporting args/flags in them?

See #1288 (comment), sort/rsort could be turned into one command.

How args/flags should be displayed in prompt autocomplete (UX/UI)?

A completer could provide them as options

How args/flags should be documented (discoverability story)?

Kakoune has a nice way of showing these:

flags

@dead10ck
Copy link
Member

It's very possible switches are out of scope for this PR (at least in a general way) so consider the following somewhat off topic, but I have thought it might make sense to pull in a CLI arg parsing lib to make use of in commands. I know there was resistance to this in the past for the main binary, but I think it would be a stronger justification to pull in a lib if it were used throughout the command palette.

@zummenix
Copy link
Contributor Author

A completer could provide them as options

:write accepts optional file name. I think there should be a mechanism to show switches in completer menu so they look distinct from file names.

Kakoune has a nice way of showing these:

I like it!

It's very possible switches are out of scope for this PR

Definitely :)

In overall I'm still on the fence about switches in commands but I see how they could be useful. I suggest you to open separate issues/discussions to iron out details how they should look like in UI and to build a vision for the feature as a whole. Also I'm thinking if there will be some kind of scripting language in the future, will it cover this usecase? For example, we could have a method write with parameters: :eval 'write(force: true, auto-format: false, method: .replace)'

The question about this PR: do we close it or I can continue with it to fix the CI error to be ready for the merge?

@dead10ck
Copy link
Member

I'd like to see these commands merged, but I'd wait for @archseer's input for this PR and what we should do for names and/or switches.

@kirawi
Copy link
Member

kirawi commented Nov 28, 2022

By the way, be sure to run cargo xtask docgen and commit the changes.

@the-mikedavis
Copy link
Member

I don't think it's necessary to add commands for this since there's a workaround: you can :set auto-format false, :write, and then :set auto-format true when you want auto-formatting again.

@zummenix
Copy link
Contributor Author

By the way, be sure to run cargo xtask docgen and commit the changes.

Yeah, I know, thanks! I was just waiting for the final decision.

@zummenix zummenix closed this Dec 12, 2022
@zummenix zummenix deleted the #4853-write-without-auto-format branch December 12, 2022 17:38
@lukepighetti
Copy link

Disappointed that this didn't get merged. Many editors have this in their command palette and the :set auto-format false, :write, :set auto-format true workflow is not discoverable

@kirawi
Copy link
Member

kirawi commented Jan 18, 2023

With #4423, it could be done. However, it would depend on if the maintainers are okay with bundling some command sewuences.

@andradei
Copy link

@the-mikedavis this to my config, but it doesn't work. I have to run the 3 commands manually...

[keys.select]
S = [':set auto-format false', ':write', ':set auto-format true']

@mwyvr
Copy link

mwyvr commented Oct 10, 2024

Was just looking for just this, and discovered as @andradei that the three-command approach wasn't working, auto-format continued. I don't need it often but when adding a new package to a Go project, lsp complains and removes the import line.

I settled on another route: set a keybinding to ":toggle-option auto-format".

@andradei
Copy link

@mwyvr That's the workaround I'm using. I think the next version of configuring helix will solve this and much more. Until then, it is what it is.

marcusandre added a commit to marcusandre/dotfiles that referenced this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write without formatting
7 participants