Skip to content

Conversation

@naserca
Copy link

@naserca naserca commented Mar 5, 2025

  • The warning is trying to do the job of mix xref from a place of incomplete information. As the warning itself implies, it may be inaccurate.
  • This is why ecto and phoenix and oban and other libs who expand like this don't try to warn in cases like "set a module attribute, use it."
  • In fact, the example in the comments here doesn't actually cause a compile-time dep.
  • In the case of false positives like this, this warning scares the engineer unnecessarily. We are experiencing this currently :)
  • Instead the canonical libraries (ecto, etc) leave it to the compiler and mix xref to let the engineer know if there's a compile-time dep problem.

@coveralls
Copy link

coveralls commented Mar 6, 2025

Pull Request Test Coverage Report for Build 222163f205c6c02dda2540a463b9d6e52035a5f1-PR-123

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 82.796%

Totals Coverage Status
Change from base Build e84dea09575afff8710bab7ada75e1262ddeb082: 0.4%
Covered Lines: 308
Relevant Lines: 372

💛 - Coveralls

@mathieuprog
Copy link
Owner

What do you think of making the warning optional and in the log say someth like: if you do not wish to see this warning due to false positive, do X?

@naserca
Copy link
Author

naserca commented Mar 6, 2025

IMO even a flag isn't appropriate since no canonical elixir libraries do this. Since one can't determine during compilation of the file itself if a dependency will be troublesome, other libs leave it to mix xref, the compiler, and the engineers working on a proj to resolve any issues.

But! It's your library, so I'm fine to add a flag if you'd prefer. Let me know!

@mathieuprog mathieuprog merged commit a896e4e into mathieuprog:master Mar 6, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants