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

Better prefill and completion for :GoRename #1465

Merged
merged 3 commits into from
Oct 14, 2017
Merged

Better prefill and completion for :GoRename #1465

merged 3 commits into from
Oct 14, 2017

Conversation

arp242
Copy link
Contributor

@arp242 arp242 commented Sep 21, 2017

  1. Repurpose g:go_gorename_prefill to be an expression, which can
    transform the value that is pre-filled to the most common forms.
    The default is to use camelCase when the identifiers starts with a
    lowercase letter, and CamelCase when it starts with an uppercase
    letter, which sounds like a reasonable guess to me.
  2. Add completion support, so you can use :GoRename <Tab> to get the
    most common options.

Both of these changes make it a bit easier/faster to use, especially if
you need to rename a bunch of identifiers in a project that doesn't
follow Go standards (like I had to do last week).

@bhcleek
Copy link
Collaborator

bhcleek commented Oct 14, 2017

I like this; it's a good usability improvement.

The defaults don't seem to work as you describe, though; I get FooBar as the default regardless of whether I'm renaming fooBar or FooBar.

Can we rename camelcaseExported to pascalcase or exportedcase? Pascal case is the name I've most often encountered for the the style where the first character of an identifer is capitalized. In the docs, you could probably just forego giving the cases a name, and instead just refer to toggling whether the first character is capitalized:

By default it toggles the capitalization of the first character of the identifier.

I don't think we should break people that currently have g:go_gorename_prefill; can we make it backward compatible with 1 and 0 so that the old behavior and semantics are retained for at least one release so that we can give people some warning? (Showing an informational message when renaming if the g:go_gorename_prefill is set to 0 or 1 would seem appropriate if you're feeling enthusiastic).

Can you go ahead and update the CHANGELOG.md simultaneously?

1. Repurpose `g:go_gorename_prefill` to be an expression, which can
   transform the value that is pre-filled to the most common forms.
   The default is to use camelCase when the identifiers starts with a
   lowercase letter, and CamelCase when it starts with an uppercase
   letter, which sounds like a reasonable guess to me.
2. Add completion support, so you can use `:GoRename <Tab>` to get the
   most common options.

Both of these changes make it a bit easier/faster to use, especially if
you need to rename a bunch of identifiers in a project that doesn't
follow Go standards (like I had to do last week).
- Rename `camelcaseExported` to `pascalcase`.
- `g:go_gorename_prefill=1` is an alias for the default, for
  compatibility reasons.
@arp242
Copy link
Contributor Author

arp242 commented Oct 14, 2017

The defaults don't seem to work as you describe, though; I get FooBar as the default regardless of whether I'm renaming fooBar or FooBar.

Hm, it seems to work for me; with this file:

package a

var (
    FooBar = "x"
    fooBar = "y"
)

if I press :GoRename<CR> on FooBar it will prompt with vim-go: rename 'FooBar' to: FooBar, and on fooBar it will prompt vim-go: rename 'fooBar' to: fooBar.

Maybe you did something different?

Can we rename camelcaseExported to pascalcase or exportedcase?

Yeah, this sounds better 👍

I don't think we should break people that currently have g:go_gorename_prefill

Sure, this sounds reasonable.

Can you go ahead and update the CHANGELOG.md simultaneously?

My general strategy with adding ChangeLog entries is to do this right before merging, or after merging when merging multiple PRs. The reason for this is that otherwise it's a forever source of merge conflicts and generally just annoying to deal with.

I don't have super-strong feelings on this or anything; maybe best to discuss on Slack how to best deal with this in future PRs?

@arp242
Copy link
Contributor Author

arp242 commented Oct 14, 2017

In the docs, you could probably just forego giving the cases a name, and instead just refer to toggling whether the first character is capitalized:

By default it toggles the capitalization of the first character of the identifier.

This isn't quite what it does, though. Maybe that was actually also the confusion about your other comment?

It tries to preserve the capitalisation of the first character, this matters because otherwise renaming something like FOO_BAR to fooBar will make it unexported, which may break things.

I've (hopefuly) clarified the docs a bit, let me know what you think.

@bhcleek
Copy link
Collaborator

bhcleek commented Oct 14, 2017

Yes, that was the source of my confusion. Thank you for updating the docs!

I'm good with postponing the CHANGELOG.md until post-merge; I'm short on time now, but will try to update it later if you don't get to it first.

LGTM.

@bhcleek bhcleek merged commit 112ea90 into fatih:master Oct 14, 2017
@arp242 arp242 deleted the rename-complete branch October 14, 2017 19:23
@arp242
Copy link
Contributor Author

arp242 commented Oct 14, 2017

Thanks; I've updated the ChangeLog.

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