-
Notifications
You must be signed in to change notification settings - Fork 325
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
Add --warnings-as-errors flag for non-zero exit code (continued) #1564
Add --warnings-as-errors flag for non-zero exit code (continued) #1564
Conversation
a406792
to
df1758b
Compare
I gave this a spin tonight, but noticed that my project (which produces ex_doc warnings) still returns with an exit code of Perhaps it's just that the types of warning that I have hasn't been instrumented yet? Repro details here. |
Hi @christhekeele, I was working on this PR and I also noticed this due to some new tests failing. |
I just read through the description of the global state issue. My idea for tackling it would be to:
That should work. |
Just wanted to let you guys know I am working on finishing this PR. |
@eksperimental I'd forgotten this is the thread where I brainstormed the above approach above, but in fact ended up needing to do something similar last week for a test helper in another project! It worked well, and FWIW my solution essentially looks like what's suggested above, something like: defmodule ExDoc.WarningCounter.TestHelpers do
defmacro isolated_warning_counter(do: code) do
quote location: :keep do
temp_table = :ets.new([:set])
Process.put({:ex_doc, :warnings_counter}, temp_table)
try do
unquote(code)
after
Process.delete({:ex_doc, :warnings_counter})
:ets.delete(temp_table)
end
end
end
end
defmodule ExDoc.WarningCounter do
def increment do
warnings_table = Process.get({:ex_doc, :warnings_counter}, ExDoc.WarningCounter)
end
end
defmodule ExDoc.WarningCounterTest do
use ExUnit
import ExDoc.WarningCounter.TestHelpers
test "some thing" do
isolated_warning_counter do
test_goes_here
end
end
end The If |
Hi @christhekeele , thank you for the advice. Unfortunately I think it will not work because because ExDoc launches multiple processes, so working on a process level does not work. I can share what I have so far so you see the real code and how it fails. |
@christhekeele I think we need to work on a caller level. |
@eksperimental Sure, I'd love to see your code—though I don't notice any recent pushes to this branch. I can't quite visualize the callers solution you are proposing with something more concrete 😉 |
c154edf
to
4573f25
Compare
This is done by implementing ExDoc.WarningCounter using a GenServer and :counters
984e098
to
a0e2944
Compare
Hi @christhekeele @elixir-lang/ex-doc There's one thing that is left. The error message says the docs failed to build due to warnings, which is not true. The docs are generated: Also, what should the behavior be given |
Hey @eksperimental! Sorry for the delay, just got back from a vacation.
I feel like this feature should go live and be tested in existing workflows before modifying the undocumented (outside of this error message) behaviour of what happens to artifacts during build, because I'm sure existing workflows out there implicitly rely on it. So I'd prefer we modify the error message be to be more accurate in the event of this failure mode, and propose a potentially-breaking temporary artifact management process after merge.
I think we can defer this decision to after-merge based on real-world usage if we do the above.
I'd be happy to give the code a more detailed review, but cannot commit to finding the time for another few weeks, I'm afraid. Busy summer at work and in life! If you want to test against the situation that led me to comment on your efforts to date, per this note, you should be able to do this in your shell: git clone git@github.com:christhekeele/matcha.git
cd matcha
git checkout a4d02a9
# Edit this line:
# https://github.com/christhekeele/matcha/commit/a4d02a966c48233b244e0249a9ddeabaadc10b35#diff-dfa6f4ed74c90e5d4fda283d547d366586e690387289bcfd473e3fa5f9ace308R110
# to point the `ref` to your latest commit
mix docs --warnings-as-errors && echo success $? || echo failure $?
# Expect to see `failure 1` |
Hi @christhekeele
We also need to review if the following warnings recently introduced to ExDoc, still qualify as warnings.
|
I just tested this on https://github.com/ScenicFramework/scenic and it successfully caught the warning that currently exists on the
Although I agree that the warnings are a little contradictory because the docs are still generated (which is counter to how --warnings-as-errors behaves when compiling elixir code). |
May I ask what is needed to be complete with this PR? |
Supervisor.start_link([ExDoc.Refs], strategy: :one_for_one) | ||
children = [ | ||
ExDoc.Refs, | ||
ExDoc.WarningCounter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of introducing some global state, I wonder if you could create a counter and store it in the config
. And then you call warning(config, ...)
which will bump the counter. This means it is naturally async and you don't need the additional process. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same vein, perhaps mix docs
would return https://hexdocs.pm/mix/1.15/Mix.Task.Compiler.Diagnostic.html structs? Could be useful for IDEs? Or since ExDoc is not a compiler it’s a bad idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't see how they could be used if it is not a compiler :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing config simplifies a lot the implementation.
The problem I am facing now is that there are functions that generate a warning and have no notion of config,
such as ExDoc.Markdown.Earmark.to_ast/2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to either:
- pass an optional function to them to configure how logging happens
- have those functions return their warnings so we can emit them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d do the latter, return {ast, %{warnings: warnings}}
or something like that.
I think such APIs that return instead of emit warnings are nice because we can test without capture_io and thus async. In this particular case since it’s not useful to return these warnings from mix docs
, what we can also do is push all ExDoc warnings via some ExDoc.Utils.warn, inside call Application.put_env(:ex_doc, :warnings_emitted, true), check for this app env at the end of the run and call it a day. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The counter will tell us if warnings were emitted or not. So having a config plus function return should be enough!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will work on this solution and let you know when it is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you ❤️
This PR now has a bunch of conflicts because we've made a lot of internal changes. Sorry about that! On the flip side, I believe implementing this feature should be now much easier. I have added a One way to go about it is:
defmodule ExDoc.Utils do
def warned? do
:persistent_term.get({__MODULE__, :warned?}, false)
end
end
unless warned?() do
:persistent_term.put({__MODULE__, :warned?}, true)
end
PR welcome! |
The initial solution had a global state and it was simple. I will re-implement it using |
I was actually just looking for this option again to throw into our CI pipeline to make sure we keep everything up to date. |
✅ Done: #1831 |
Closes #1411
Closes #1294
PR Originally published in #1412
I'm picking this feature up from it was left off.
The only thing to solve is the global state issue mentioned by @milmazz in #1412 (review)