Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

Better error handling for Elixir #1343

Closed
wants to merge 1 commit into from
Closed

Conversation

aglyzov
Copy link

@aglyzov aglyzov commented Mar 2, 2015

  • changed compiler to 'elixirc --ignore-module-conflict'
  • added two more errorformats to match errors and warnings

- changed compiler to 'elixirc --ignore-module-conflict'
- added two more errorformats to match errors and warnings
@lcd047
Copy link
Collaborator

lcd047 commented Mar 2, 2015

On a quick look:

  • using elixirc instead of elixir and not changing checker's name is wrong
  • -o /tmp is wrong
  • your errorfromat is probably wrong too.

What problem are you actually trying to solve? @xandox, @thomas-holmes: any comments?

@aglyzov
Copy link
Author

aglyzov commented Mar 2, 2015

@lcd047 how did you infer my errorformats are wrong? the code has samples of real errors/warnings that are being matched (obviously they didn't match before).

@aglyzov
Copy link
Author

aglyzov commented Mar 2, 2015

@lcd047 and about -o /tmp – yes, it's too Unix-centric, I know. what do you propose to fix this?

@lcd047
Copy link
Collaborator

lcd047 commented Mar 2, 2015

how did you infer my errorformats are wrong?

You're relying on %C being tried in order until one of them matches. That's not how errorformat works. What's going on is way more involved than that, and frankly it isn't worth spending time digging through Vim sources to understand it. If you need to deal with the kind of stuff you mention in your commit the solution is to normalise it in a preprocess function. See autoload/syntastic/preprocess.vim for examples.

and about -o /tmp – yes, it's too Unix-centric, I know.

Actually, it doesn't even work in UNIX. If two users are trying to check files with the same name at the same time, one of them is going to get an EPERM, an EEXIST, or something similar. :)

Another problem is that it leaves temporary files behind.

what do you propose to fix this?

If you really need a temporary directory, there's an example of how to do it in, say, the clisp checker. But I'd really prefer to avoid that if possible.

Still, my suggestion would be to begin by answering my question: what problem are you actually trying to solve?

@aglyzov
Copy link
Author

aglyzov commented Mar 2, 2015

You're relying on %C being tried in order until one of them matches. That's not how errorformat works.

  1. it works okay for me (and I read vim errorformat docs before writing these)
  2. if you're so sure it doesn't work properly then pls provide a real Elixir error that doesn't work with this efm

what problem are you actually trying to solve?

I noticed that the majority of errors and warnings of the latest Elixir compiler didn't catch by an up-to-date Syntastic so I dug into and found out the errorformat wasn't even anticipating those.

@lcd047
Copy link
Collaborator

lcd047 commented Mar 2, 2015

  1. if you're so sure it doesn't work properly then pls provide a real Elixir error that doesn't work with this efm

Oh I don't need to, the fact that your errorformat is unmaintainable is reason enough to reject your PR. But I'd still like to hear from other people that actually use this checker about the value of the change you suggest, that is, using elixirc --ignore-module-conflict for checks.

@lcd047
Copy link
Collaborator

lcd047 commented Mar 9, 2015

Ok, I took a quick look at this. Elixir error handling is a clusterfuck that almost goes out of its way to be parsing-hostile. There's little hope syntastic will ever get to match everything correctly, regardless of how much time you waste fiddling with errorformat (in particular, your %+C %m matches general exceptions). I'd say this checker should not have been added to syntastic to begin with, and should probably be removed in a future version. As such, and considering the overwhelming feedback above, :) I'll settle for 69d20ef. Sorry about that.

As for elixirc --ignore-module-conflict, it turns out elixirc is just a small script around elixir, so you can achieve the same effect by just setting g:syntastic_elixir_elixir_args. Sorry about that, too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants