-
Notifications
You must be signed in to change notification settings - Fork 89
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
Method Params #47
Comments
I managed to extract parameters with vim-surround using
I would like to have something like this in the end:
Nothing |
Well, theoretically, it would be possible, but right now, splitjoin breaks down options, and not arguments. So basically: method_call "one", "two", three: "four", five: "six"
# splits into
method_call "one", "two", {
three: "four",
five: "six",
} This is probably the most common split you'd want. Now, it would be perfectly possible to also split the positional arguments, but in the above case, it wouldn't really look that good: method_call(
"one",
"two",
{
three: "four",
five: "six",
}
) Or something like this, anyway. Of course, if you don't have options in there, it's a lot better: method_call(
"one",
"two"
) So it would be possible to implement this, but only if there are no options in the method call. Do you think this makes sense? |
I've pushed a simple implementation of splitting to the |
Thanks! I always miss that, when line gets too long first I put params on a separate line and if gets too long again – put them vertically. |
Hmm, almost there. Few suggestions though... First a bug: the following line cannot be split regardless of cursor position: params.require(:article).permit(:title, :meta, :slug, :lead, :body, :category_id) In regards to functionality, here is how it would work in my world: ▋.permit(:title, :meta, :slug, :lead, :body, :category_id)
▋.permit(
:title, :meta, :slug, :lead, :body, :category_id
)
.permit(
▋:title,
:meta,
:slug,
:lead,
:body,
:category_id
) ... if you get my drift. Here is what it does now: .permit(:title,
:meta,
:slug,
:lead,
:body,
:category_id) I believe I am offering a superiour scheme that also adds two different splits. Plus I find super-indented listings weird as always try to use closer 80 chars per line. I saw it splits like this: xxxxxxxxxxx (:xxxxxxx,
:xxxxxxx) I'd rather prefer xxxxxxxxxxx (
:xxxxxxx,
:xxxxxxx
) Saw another strange split, instead of starting from innermost parens it added new ones around: ▋redirect_to article_path(@article, locale: @article.translations.first.locale)
redirect_to article_path(
▋@article, locale: @article.translations.first.locale
) And then redirect_to article_path(
▋@article,
locale: @article.translations.first.locale
) I get: redirect_to (
article_path(@article, locale: @article.translations.first.locale)
) Thank you very much, I hope you will find the suggestions reasonable if having two additional ways of splitting is not too much to ask. I recently started enjoying Vim and got almost all bases covered, there are some bumps on the road that I hope to rectify. |
Sorry just read your post, didn't notice it at first. I think we're talking about the same thing. It just need to start splitting from the inside of parens. In regards to your examples: method_call "one", "two", three: "four", five: "six"
# splits into
method_call "one", "two", {
three: "four",
five: "six",
} ^ Perfect.
^ Looks strange? Actually not at all when you have nested Besides, who cares? Those who don't want automation can align everything by hands. I always use Code that was splitted should not move after |
For the record: after splitting long-indent style code stays still after params.require(:permission).permit(▋:title, :action, :subject_type, :subject_id, :own)
def permission_params
params.require(:permission).permit(▋:title,
:action,
:subject_type,
:subject_id,
:own)
end Now I do But then I would need to split the the arguments line at commas anyway... |
Ah, here you go! Before: params.require(:permission).permit(▋:title, :action, :subject_type, :subject_id, :own)
params.require(:permission).permit(
:title,
:action,
:subject_type,
:subject_id,
:own
) Perfect! I don't know if this can be considered closed now or you might want to add an additional splits for parens and comma separated vars. |
Could support for JS methods be added as well? |
Okay, let's start from the top
I'm not a big fan of this kind of formatting, but it seems to be common. The params.require(:permission).permit(:title,
:action,
:subject_type,
:subject_id,
:own)
params.require(:permission).permit(
:title,
:action,
:subject_type,
:subject_id,
:own
) The way I imagine it is just like the settings of hash splitting: Regarding this example: redirect_to article_path(@article, locale: @article.translations.first.locale)
redirect_to (
article_path(@article, locale: @article.translations.first.locale)
) The problem here is that the plugin looks for the outermost method call (I think. I've forgotten the details, to be honest). This makes sense in most cases, since there's not much of a reason to split a nested method call without splitting the outer one. In this case, it has a method call It is a bit odd, but it's hard to do it the other way around, and would break many other use cases, I think. One feature I've been sitting on for a while is splitting whatever's marked in visual mode (#32), which would make such ambiguities solvable, at least -- if it doesn't do what you want, simply mark your target and split that. Theoretically, that is. Plus, I have yet to implement it properly.
That may be so, but it would be hard to decide when to split options only, and when to split arguments. The "looks strange" description is to say: "doesn't look like canonical ruby". Yes, if you have a ton of complex fields in the method call, maybe it would look okay like this (although maybe you want to extract some variables instead, but that's a different topic). But in most cases, you're going to have one or two positional arguments that are variables and a ton of options, so this kind of style would be much, much better (I think): method_call(one, two, {
three: "four",
five: "six",
seven: "eight",
nine: "ten",
}) But you have only one mapping to split, it's kind of hard to make the guess of which one would look better. I'd rather tackle the more common case and leave it to the user to adjust the unusual situations. On a related note, maybe I can interest you in my "extract variable" script? :) Bottom line, I'll add an option to the code to split in either of the above two ways and I'll merge it to master (if you confirm that you're happy with it). I'll only split arguments if there is no option hash at the end -- if there is, that one gets split instead, since it should be a more common use case. Considering that the only cases in which I've ever had really long argument lists are @jordwalke, I'll see what I can do. |
I've just pushed the new option and some minor fixes to the addition of round brackets to the branch. @firedev, could you give it a final test before I merge it into master? |
Sorry I was on a trip in a place where internet is a luxury, will check it now. In regards to the script, what is the intended behaviour? I can figure out it extracts and inlines variables somehow, would be great to have some pointers. |
Here are my observations so far. I am not 100% sure they are even related to this branch, but anyway. Don't want to be nitpicky but with the introduction of Rubocop all code I've written 6 months ago requires some improvements. Not that I didn't know about styling back then, but without constant nagging it's easy to disregard style guide violations here and there.
FactoryGirl.create(:inquiry, state: state) Expected: FactoryGirl.create(:inquiry, {
state: state
}) Actual: FactoryGirl.create(:inquiry,{ # no space after comma
state: state, # extra comma
})
expect(page).to(have_content('long string goes here')) Expected: expect(page).to(
have_content('long string goes here')
) Actual: Does nothing. |
I just pushed a fix for example 1 (this has been a bug for a while, actually, I've noticed it myself, but never had the time/energy/attention to investigate). As for example 2, I think you need to put Could you double-check this? |
Sorry, I was sick the whole week didn't do much work. Will do, thanks! |
Ah, yes, regarding the "extract variable" script: Marking a piece of code (on a single line) and then hitting Pressing The mnemonics for the mappings are "o" for "out", "i" for "in", and the "s" is just a prefix I don't use. They can be changed in the first two lines. The patterns for a variable definition for both mappings are also customizable using buffer-local variables, like here: https://github.com/AndrewRadev/Vimfiles/blob/1397687471612b7e1582648488c81740695ba460/ftplugin/go.vim#L9-10. |
|
Take a look at the top of the script: xmap so :<c-u>call <SID>ExtractVar()<cr>
nmap si :<c-u>call <SID>InlineVar()<cr> The top mapping is in visual mode (it's not |
I've merged the code to master, since I've been using it for a while and it seems mostly stable (I just made a fix for a spacing issue, but other than that). I'll close the issue for now, but feel free to reopen if you find problems. |
Thank you, sorry for the lack of feedback, I've moved from refactoring to writing new code in the last week or two and couldn't collect enough data to report. |
No worries, I'm not exactly quick with work on these plugins anyway. If you run into any issues, just let me know. |
It would be great if split join could do something like extracting params:
Related to tpope/vim-surround#140
The text was updated successfully, but these errors were encountered: