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

Unify formatting of some common expansion errors #5797

Merged
merged 4 commits into from
Feb 20, 2017
Merged

Unify formatting of some common expansion errors #5797

merged 4 commits into from
Feb 20, 2017

Conversation

whatyouhide
Copy link
Member

These errors can now be formatted via either elixir_expand:format_error/1 or
elixir_clauses:format_error/1.

These errors can now be formatted via either elixir_expand:format_error/1 or
elixir_clauses:format_error/1.
format_error(missing_do_or_after_in_receive) ->
"missing do or after keyword in receive";
"missing :do or :after in \"receive\"";

format_error(missing_keyword_in_try) ->
Copy link
Member

Choose a reason for hiding this comment

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

keyword –> option?

Copy link
Member Author

@whatyouhide whatyouhide Feb 20, 2017

Choose a reason for hiding this comment

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

Great catch. Was thinking, what if we unify this error as well and have something like

format_error({missing_options, Kind, Opts}) ->
  StringOpts = 'Elixir.Enum':map_join(Opts, <<"/">>, fun 'Elixir.Macro':to_string/1),
  io_lib:format("missing ~ts in \"~ts\"", [StringOpts, Kind]);

Wdyt? We would unify try and receive as well this way.

Copy link
Member

@lexmag lexmag Feb 20, 2017

Choose a reason for hiding this comment

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

I believe Elixir.Enum won't be available on that step, we could use Erlang alternative.
I think it makes sense to unify like that. 👍

Copy link
Member

Choose a reason for hiding this comment

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

We can also perhaps generalize missing_do to missing_option?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lexmag definitely, that's what I meant. :)

@lexmag lexmag self-requested a review February 20, 2017 15:51
Copy link
Member

@lexmag lexmag left a comment

Choose a reason for hiding this comment

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

Wonderful. 💛

@whatyouhide whatyouhide merged commit 03a16c7 into elixir-lang:master Feb 20, 2017
@whatyouhide whatyouhide deleted the unify-expansion-errors branch February 20, 2017 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants