Skip to content

Conversation

arrdem
Copy link
Contributor

@arrdem arrdem commented Mar 26, 2016

This causes clojure-forward-logical-sexp to skip over , when for
instance computing vertical alignments.

Fixes #367

This causes clojure-forward-logical-sexp to skip over , when for
instance computing vertical alignments.

Fixes clojure-emacs#367
@Malabarba
Copy link
Member

Well, I guess that was easier than expected. 😄
Could also add a test for this in the alignment tests? It's very easy to write. Just copy one of the other tests in there, and replace the code string with an example that failed before but works now.

@Malabarba
Copy link
Member

And thanks a ton btw!

@arrdem
Copy link
Contributor Author

arrdem commented Mar 26, 2016

Yeah I'm honestly kinda surprised that this is all it took even in some fairly torturous case...

2016-03-25-204426_251x86_scrot

@arrdem
Copy link
Contributor Author

arrdem commented Mar 26, 2016

I don't think this is right however
2016-03-25-204523_311x87_scrot

@arrdem
Copy link
Contributor Author

arrdem commented Mar 26, 2016

This is also pretty tortured....

2016-03-25-204641_213x114_scrot

@arrdem
Copy link
Contributor Author

arrdem commented Mar 26, 2016

Also isn't right... not sure what's going on here.

2016-03-25-205058_615x105_scrot

@Malabarba
Copy link
Member

Hmm. Now that I think about it... The current code probably assumes that non-logical sexps come before the logical sexp, which is not true for commas.
I don't think we need to change that, though (commas aren't really sexps anyway). It should be enough to just (skip-chars-forward ",") after moving over a logical sexp. This won't cover some of the more tortuous cases you tried above, but it should cover the case of a single comma after a sexp, which is really the only one I'm concerned with.

@arrdem
Copy link
Contributor Author

arrdem commented Mar 27, 2016

Okay. These two together seem to do the job better.
2016-03-26-232339_241x65_scrot

@Malabarba Malabarba merged commit 786c74d into clojure-emacs:master Mar 28, 2016
@Malabarba
Copy link
Member

Merged! I also added a bit of code so that clojure-align will automatically cleanup excessive commas.

@arrdem
Copy link
Contributor Author

arrdem commented Mar 28, 2016

Did you test this at all for yourself? I'm concerned that it has fairly significant limitations from mucking this afternoon 😞

@Malabarba
Copy link
Member

I did, but I also changed the code a bit, which might have overcome the limitations. Have you tried the version on master?

@arrdem
Copy link
Contributor Author

arrdem commented Mar 28, 2016

No I've been in hospital all day. I'll check it out tomorrow.

@Malabarba
Copy link
Member

👍 Let me know when you get a chance.

@arrdem
Copy link
Contributor Author

arrdem commented Mar 28, 2016

LGTM. Thanks Artur!

2016-03-28-102731_583x97_scrot

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