Skip to content

Add support for Elixir arrays #163

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.

Sign up for GitHub

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

Jump to bottom
Merged
AndrewRadev merged 3 commits into from
Jul 21, 2020
Merged

Add support for Elixir arrays #163

AndrewRadev merged 3 commits into from
Jul 21, 2020

Conversation

@frm
Copy link
Contributor

@frm frm commented May 14, 2020

This PR adds support for array splitting and joining in Elixir.

Works with the following examples:

[a, b, c]

[
  a,
  b,
  c
]

[a, b, c | d]

[
  a,
  b,
  c 
  | d
]

[a: 1, b: 2, c: %{a: 1, b: 2}]

[
  a: 1,
  b: 2,
  c: %{a: 1, b: 2}
]

[a: 1, b: 2 | [c: %{d | e: 1}]]

[
  a: 1,
  b: 2 
  | [c: %{d | e: 1}]
]

[1, 2, 2 |> Kernel.+(1) | [4]]

[
  1,
  2,
  2 |> Kernel.+(1) 
  | [4]
]

I did manage to find an edge case where this doesn't work:

[a: 1, b: 2, c: %{d | e: 1}]

[
  a: 1,
  b: 2,
  c: %{d 
    | e: 1}
]

However, to handle this it's necessary to build a parser and given it's such an uncommon edge case (updating a map when declaring the last element of an array), I assumed it wouldn't be worth the effort.

@frm
Copy link
Contributor Author

frm commented May 14, 2020 •
edited
Loading

On a sidenote: as a byproduct of this, I managed to add some missing tests for Elixir and I'm currently working my way through maps and arrays via sigils.

I'll be making a few more PRs adding those.

AndrewRadev
AndrewRadev requested changes May 15, 2020
View reviewed changes
Copy link
Owner

@AndrewRadev AndrewRadev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for you work! This looks good to me, although I did find issues in some edge cases. Could you take a look?

I also see there's a build failure, which I think happens because the build runs on Vim 7.4.52, which is quite old. I wouldn't be surprised if it's missing Elixir support, which would mean indentation doesn't work. It's a pain in the ass, getting newer filetypes to work for the build. I mostly mark them as pending when run in TravisCI, or I remove all indentation in the assertions, both of which are clumsy solutions.

If you can find a way to fix the build with either of these methods, go ahead, but I don't consider it important -- I can handle this later. I'm starting to think I could just add the extra language support via git submodules to the test setup (like I'm doing here).

@frm
Copy link
Contributor Author

frm commented May 19, 2020

Thanks for reviewing! I'll be handling these later today.

I did have some issues running the tests with indentation (especially adding tests to existing do-blocks on a different branch) because macvim also has some issues with indentation. It'd really help if I could add a submodule for vim-elixir.

In fact, it already depends on it since the delimiters used in sp#elixir#SplitArray() are in fact provided by vim-elixir. I actually forgot to mention in the PR description, but I assumed that dependency since I'm 99.99% certain someone working with Elixir in Vim to the point they'd need to use splitjoin, would be using vim-elixir. I think a similar dependency already exists with the Ruby parser and vim-ruby.

@AndrewRadev
Copy link
Owner

AndrewRadev commented May 19, 2020 •
edited
Loading

I assumed that dependency since I'm 99.99% certain someone working with Elixir in Vim to the point they'd need to use splitjoin, would be using vim-elixir. I think a similar dependency already exists with the Ruby parser and vim-ruby.

Yes, there's a lot of dependencies, to Rust as well. A couple of days ago, I decided to vendor these in specs, and this seems to be working fine. Here's the lines that load the plugins:

splitjoin.vim/spec/spec_helper.rb

Lines 14 to 16 in 99d1200

# Up-to-date filetype support:
vim.prepend_runtimepath(plugin_path.join('spec/support/rust.vim'))
vim.prepend_runtimepath(plugin_path.join('spec/support/vim-javascript'))

And here's the .gitmodules file: https://github.com/AndrewRadev/splitjoin.vim/blob/master/.gitmodules

So, you could do the same for vim-elixir, but I can do that for you as well -- if you'd like, focus on your fix and I'll get the tests running afterwards.

@frm frm force-pushed the frm/elixir-arrays branch from f7a2333 to ee3f263 Compare July 20, 2020 23:19
@frm
Copy link
Contributor Author

frm commented Jul 20, 2020

Sorry it took me a couple of months but finally got to handle this. I think these scenarios should now all be handled :)

AndrewRadev
AndrewRadev approved these changes Jul 21, 2020
View reviewed changes
Copy link
Owner

@AndrewRadev AndrewRadev left a comment

Choose a reason for hiding this comment

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

I recently merged another PR that added some Elixir specs, so that might have created some conflicts for you. Sorry for that! 😅

In any case, the cases do seem to be handled and the code looks good -- I'll go ahead and merge. Thank you for your work!

@AndrewRadev AndrewRadev merged commit 237373f into AndrewRadev:master Jul 21, 2020
@frm frm deleted the frm/elixir-arrays branch July 21, 2020 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndrewRadev AndrewRadev AndrewRadev approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@frm @AndrewRadev