Add --warnings-as-errors flag for non-zero exit code #1412
Conversation
|
Amazing! What if we call it —warnings-as-errors? |
kdawgwilk
force-pushed
the
kaden/strict_flag
branch
from
567a480
to
c57345d
Compare
kdawgwilk
changed the title
|
Are we okay with exiting after the first round of warnings are encountered? Or do we want to spit out all of the warnings before we exit? The easier approach would be to exit early but it may be frustrating to:
But due to this being a new feature having the early exit is still an improvement and could be improved later to show all warnings. EDIT: Do we also want to support configuring this in |
|
Ideally we would not fail early. We would have a process in ExDoc's supervision tree (it can be an Agent) which will store if warnings were emitted. If so, you abort at the end of the |
kdawgwilk
force-pushed
the
kaden/strict_flag
branch
from
c57345d
to
e6093e0
Compare
kdawgwilk
force-pushed
the
kaden/strict_flag
branch
from
e6093e0
to
57c9069
Compare
|
@kdawgwilk ping! I think this is pretty close, feel free to change from a draft to ready for review so we can merge it soon. (Of course no rush!) |
kdawgwilk
force-pushed
the
kaden/strict_flag
branch
from
57c9069
to
cdacc39
Compare
|
Struggling to figure out how to write some good tests for this, other than that I think it is mostly ready to go |
|
@kdawgwilk ping. :) I think the tests are pretty good as is, no need for more coverage imho. |
kdawgwilk
force-pushed
the
kaden/strict_flag
branch
2 times, most recently
from
c1f7f2c
to
3d0dcc7
Compare
|
@wojtekmach I think I have enough tests now just having a hard time getting them to behave with the exit code, have 1 left that is still failing because there are no actual warnings where I am expecting a warning so I can assert it exits as expected. Not sure how to use the fixtures to get a doc warning when I run from the tests. After thats figured out this should be good to go |
kdawgwilk
force-pushed
the
kaden/strict_flag
branch
from
3d0dcc7
to
81f80da
Compare
|
Just figured it out now so this should be ready for review! |
|
@kdawgwilk This looks good to me. Do you need help with something in particular? or in your opinion, are we good to go? EDIT: Quick local test seems good too. diff --git a/README.md b/README.md
index 89cf2ca0..8b59e943 100644
--- a/README.md
+++ b/README.md
@@ -252,3 +252,8 @@ The easiest way to test changes to ExDoc is to locally rebuild the app and its o
ExDoc source code is released under [Apache 2 License](LICENSE). The generated contents, however, are under different licenses based on projects used to help render HTML, including CSS, JS, and other assets.
Any documentation generated by ExDoc, or any documentation generated by any "Derivative Works" (as specified in the Apache 2 License), must include a direct, readable, and visible link to the [ExDoc repository](https://github.com/elixir-lang/ex_doc) on each rendered material. For HTML pages, a rendered material represents every single page. For PDF, EPUB and other ebook formats, it means one entry for the whole material.
+
+<pre><code>
+something
+sometjing
+</code></pre>$ mix docs --warnings-as-errors
Generating docs...
warning: ExDoc.Markdown.Earmark (warning) README.md:256 Failed to find closing <pre>
View "html" docs at "doc/index.html"
Doc generation failed due to warnings while using the --warnings-as-errors option
$ echo $status
1 |
| @@ -181,4 +181,8 @@ defmodule Mix.Tasks.DocsTest do | |||
| ] = run([], app: :umbrella, apps_path: "apps/", docs: [ignore_apps: [:foo]]) | |||
| end) | |||
| end | |||
|
|
|||
| test "accepts warnings_as_errors in :warnings_as_errors" do | |||
| assert catch_exit(run([], app: :ex_doc, docs: [warnings_as_errors: true])) == {:shutdown, 1} | |||
There was a problem hiding this comment.
🤔 Individually, this test should fail, it's assuming that the state of the WarningCounter is greater than zero when is not the case.
| end | ||
|
|
||
| test "exits with 1 with warnings" do | ||
| ExDoc.WarningCounter.increment() |
There was a problem hiding this comment.
This test is running in async mode, and you're modifying a global state here that could affect other tests, which is what happening with test/mix/tasks/docs_test.exs
There was a problem hiding this comment.
What would be the simplest way to solve this?
I have tried some but it requires a large refactoring.
There was a problem hiding this comment.
If I do a ExDoc.WarningCounter.reset() on the tests that use this global state would that work?
There was a problem hiding this comment.
If I do a
ExDoc.WarningCounter.reset()on the tests that use this global state would that work?
the issue is that since they are in separate files they could be executed concurrently.
If you execute reset in one file, it will be resetting the ones in the other tests. The only guarantee is that if you use async: false is that the tests within the same file are not going to be running asynchronously.
There was a problem hiding this comment.
Sounds like this is going to be more work than I was expecting to get added 😞 I started a new job and am no longer working with elixir day to day so I likely won't be finishing this up anymore. If someone else wants to pick it up feel free.
There was a problem hiding this comment.
Sounds like this is going to be more work than I was expecting to get added disappointed I started a new job and am no longer working with elixir day to day so I likely won't be finishing this up anymore. If someone else wants to pick it up feel free.
Sure. No worries. I was already working with your PR so I'm familiar with it.
Thanks a lot for the PR, it is almost there.
|
Would love to see this feature. |
kdawgwilk
force-pushed
the
kaden/strict_flag
branch
from
81f80da
to
dc56aae
Compare
|
No longer working with Elixir day to day 😢 so I won't be able to prioritize this for awhile unfortunately |
No worries @kdawgwilk, I can jump in from where you left off. |
|
So let's define how do we want to deal with the global state of the warnings. |
Closes #1411
Closes #1294