Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Include compiler name in the generated warnings #17

Open
axelson opened this issue Apr 28, 2020 · 7 comments
Open

Include compiler name in the generated warnings #17

axelson opened this issue Apr 28, 2020 · 7 comments

Comments

@axelson
Copy link
Contributor

axelson commented Apr 28, 2020

I think it would be helpful to include the compiler name in the warnings shown by mix compile. Currently the compiler name is shown in the VSCode output, but not in the "plain" output. This will help someone understand that the warning isn't coming from Elixir itself, but instead from a library that is installed on the project.

@sasa1977
Copy link
Owner

This will help someone understand that the warning isn't coming from Elixir itself, but instead from a library that is installed on the project.

Why is it relevant where the warning is coming from? Even if we add something like "boundary compiler" to the output, the developer doesn't necessarily know that it's a 3rd party library.

@axelson
Copy link
Contributor Author

axelson commented Sep 21, 2020

I think it's important so that the user will know where to look in order to resolve the warning. For example if they start searching in https://hexdocs.pm/elixir/ they won't be able to find more information about the warning, and will potentially grow frustrated. Whereas if the compiler name is included in the warning it will be easier to find the boundary repository and documentation.

@sasa1977
Copy link
Owner

I have some doubts about how much adding the compiler name will help, partly because boundary docs are large, and partly because the solution to the warning is not always obvious to a newcomer. For example, fixing a forbidden call from context to web will require moving some part of the code (not necessarily everything) to context. Ultimately I feel that this is something where a more experienced developer on the project will have to assist.

That being said, I agree that displaying the compiler name might help. However, I wonder if this would be better done by the mix compile task, not by boundary. This would fix the issue for all other custom compilers, and additionally it wouldn't require adding what's basically noise for UIs such as VSCode, where the compiler name is already displayed.

@axelson
Copy link
Contributor Author

axelson commented Sep 23, 2020

I agree that all warnings output from a custom compiler should include the compiler name (by being inserted by mix compile).

@cdimitroulas
Copy link

cdimitroulas commented May 7, 2021

Hi both, as a newcomer to Elixir who discovered this library I just wanted to chime in and say that I agree that having some information that the particular error is related to this library in some way would be quite useful in helping identify that this is not something from elixir itself.

I see that the discussion concluded that this should be handled by mix compile rather than boundary itself. Do we need to raise an issue in mix to make this suggestion?

@sasa1977
Copy link
Owner

sasa1977 commented May 7, 2021

I doubt that this is on the priority list of the Elixir team. So as a relatively simple assistance hack, how about something like this:

warning: forbidden reference to MySystemWeb.Endpoint
  (references from MySystem to MySystemWeb are not allowed)
  lib/my_system/user.ex:3
  See https://hexdocs.pm/boundary/Mix.Tasks.Compile.Boundary.html for details.

The link points to the docs of the boundary compiler, because it provides the concise explanation:

Verifies cross-module function calls according to defined boundaries.
This compiler reports all cross-boundary function calls which are not permitted, according to the current definition of boundaries.

And then in the next sentence the reader is pointed to the Boundary module for details.

WDYT?

@gshaw
Copy link
Contributor

gshaw commented May 8, 2021

@sasa1977 I think your suggestion is a good balance in terms of addressing the problem and ease of implementation.

The only other thing that comes to mind with working with Elixir is that some error messages are often very complete and provide additional detail on how to resolve the problem right in the error. I don't think that is needed in this case though, just mentioning it as something that crossed my mind.

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

No branches or pull requests

4 participants