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

Properly report first exception #50

Closed
wants to merge 1 commit into from

Conversation

filmor
Copy link

@filmor filmor commented Feb 7, 2023

No description provided.

elbrujohalcon
elbrujohalcon previously approved these changes Feb 7, 2023
@elbrujohalcon elbrujohalcon dismissed their stale review February 7, 2023 07:40

Dialyzer is not happy with this change.

@filmor
Copy link
Author

filmor commented Feb 7, 2023

Dialyzer not being happy with it just means that elvis_core is incorrectly speced (and not correctly checked by Dialyzer?), I'll have a look. With the current implementation, the case in question (forgot to set the ruleset value) will lead to "Linting failed" without any additional information.

@elbrujohalcon
Copy link
Collaborator

Dialyzer not being happy with it just means that elvis_core is incorrectly speced (and not correctly checked by Dialyzer?), I'll have a look. With the current implementation, the case in question (forgot to set the ruleset value) will lead to "Linting failed" without any additional information.

Let's open a ticket in elvis_core to adjust the specs, then :)

@paulo-ferraz-oliveira
Copy link
Collaborator

paulo-ferraz-oliveira commented Jul 1, 2023

I don't see where elvis_core exceptions are "thrown" as {fail, [{throw, Error}|_]}, or what's catching them to transform an error into that, but I agree the tests could help developers by being more explicit in cases of e.g. badmatches.

Edit: hm, it's probably coming from

    catch
        {T, E} ->
            {error, {T, E}}

@paulo-ferraz-oliveira
Copy link
Collaborator

Once inaka/elvis_core#314 is approved+merged+tagged+released, this pull request can be updated to benefit from that and then approved+...

@paulo-ferraz-oliveira
Copy link
Collaborator

The above-mentioned pull request is in place, so this pull request can benefit from it.

@paulo-ferraz-oliveira
Copy link
Collaborator

Closing as superseded by #60. This seemed stale, and the solution seemed incomplete too. That, along with the Conflicting files, eased our choice.

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

Successfully merging this pull request may close these issues.

3 participants