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

Ternary support for ruby #22

Open
LFDM opened this issue Jan 30, 2014 · 8 comments · Fixed by #23
Open

Ternary support for ruby #22

LFDM opened this issue Jan 30, 2014 · 8 comments · Fixed by #23

Comments

@LFDM
Copy link
Contributor

LFDM commented Jan 30, 2014

Hey @AndrewRadev, would you be interesting in adding ternary support for Ruby to the built in feature set?

Here's what it does: https://gist.github.com/LFDM/8721142
And here's how its implemented: https://github.com/LFDM/splitjoin.vim/blob/ternary_support/autoload/sj/ruby.vim#L53-160

A slight change to the IfClause functions was needed, but all for the better: They now return false when there is an else case present - tests of course still green.

I'll add a PR and tests if you're interested.

@LFDM
Copy link
Contributor Author

LFDM commented Jan 30, 2014

Ah, ok there are actually specs now, I should have looked beforehand - rspec like, cool!

@LFDM
Copy link
Contributor Author

LFDM commented Jan 31, 2014

Another update: Cases can be splitjoined now as well.

@AndrewRadev
Copy link
Owner

This looks pretty good. The reason I haven't worked on ternary clauses is because I don't use them much, I guess. They're also quite easy to put in any other expression, so hard to detect. But the simple case that you handle (assignment) seems like a perfectly reasonable use case. The when handling is also pretty nice, and having case wrap all the whens is clever.

If you provide a pull request, I'm going to request some minor (I think) changes to naming and comments. In general, you've kept very nicely to my style of coding, so I don't have many complaints. I noticed there's one commented out spec, but I can take a more serious look at it after the PR.

Thank you for your work, specs included :).

@LFDM
Copy link
Contributor Author

LFDM commented Feb 1, 2014

I understand that there are people who don't like the ternary but I am not one of them. It's was almost everything else in Ruby: if you use your tools sanely, you're all good. Nesting ternaries f.e. is a no-go.
Joining them supports my workflow quite well, starting with a longer if-else construct, refactor refactor refactor, pretty short if-else left - and so far I would have manually edit it to a one-layer. I already have in in my fingers, but to be able to do it with one keystroke puts a smile on my face - vim regularily does that :)

The rest I'll comment in the PR that will pop up in a few minutes!

@AndrewRadev
Copy link
Owner

I'm going to reopen this issue, because I've run into a bit of a problem. Issue #48 illustrates it.

The problem is that the ternary pattern is very broad and takes effect even when the cursor is not on it, and even when it doesn't really make sense to split. The example there is indicative:

facilities = if facilities.flatten.map { |item| item.is_a?Facility
                                             item.id
                 else
                   item.to_i }
                 end

I think that the entire start of the line is captured as the beginning item in the ternary clause. The brackets also get captured in a weird way. I tried to scope the pattern a bit, but right now, I can't see a way to do this reliably. What should happen is that the ternary should only be split when the cursor is on it, but the start of the ternary could be basically anything :/.

Not sure what I can do about it, short of removing the ternary support altogether (for splitting at least). Any ideas?

@AndrewRadev AndrewRadev reopened this Nov 11, 2014
@LFDM
Copy link
Contributor Author

LFDM commented Dec 29, 2014

Sorry, I kinda missed the notification for that - was just thinking to do something html related and ran across this.

Will investigate at some point when I am warmed up with the code (and vimscript) in general again ;)

@LFDM
Copy link
Contributor Author

LFDM commented Dec 29, 2014

The safest thing is probably to split the ternary only in one specific case: When we are directly on the ?.

Or even better:
Split the ternary all the time as long we don't have braces on the line. When there are braces, only split on the ?. This adds a bit of inconsistency perhaps, but would at least not lead to errors and be a reasonable way to use it.

@AndrewRadev
Copy link
Owner

Being directly on the ? might be a bit tricky, it is just one character. Splitting the ternary if we don't have braces sounds a bit iffy. What if there are braces around the clause, for example, something like:

foo = (bar == baz) ? "bar" : "baz"

Also, I'm still not completely sure how detection would work. I think the problem here is that, even if you find the ?, you need to detect what comes before it, as in, what is the clause of the ?. In the above example, is it (bar == baz) or foo = (bar == baz)? Sure, in this case it's easy to tell, because we could just make a special case for =. But what about the example that breaks, for instance?

facilities = facilities.flatten.map { |item| item.is_a?(Facility) ? item.id : item.to_i }

I guess we could have a list of operators that "stop" the search, like |, or any kind of unclosed opening brace. Not sure if that'll be enough, but maybe. What do you think?

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 a pull request may close this issue.

2 participants