[#177] publish does not include docs #180
Conversation
| @@ -8,9 +8,10 @@ | |||
| -include("rebar3_hex.hrl"). | |||
|
|
|||
| -define(PROVIDER, docs). | |||
| -define(DEPS, [{default, edoc}]). | |||
| -define(DEPS, [{default, lock}]). | |||
There was a problem hiding this comment.
Removing the dependency on edoc addresses #57 in a way.
| -spec assert_doc_dir(string()) -> true. | ||
| assert_doc_dir(DocDir) -> | ||
| filelib:is_dir(DocDir) orelse | ||
| rebar_api:abort( "Docs were not published since they " |
There was a problem hiding this comment.
Since the publish command calls the rebar3_hex_docs:publish/3 function once it has already succeeded publishing the package itself, I though it would be better to use rebar_api:abort/2 for showing the user the error and aborting the execution. This will only abort the docs publishing and not the package publishing.
There was a problem hiding this comment.
Yeah I wonder if we actually want to bail on the publish 🤔 Like, if you're expecting it all in one go, then I could see where we might want to bail on the package publishing too, but I don't have strong feelings about it either.
There was a problem hiding this comment.
Yeah, I thought about this more and I really think we should strive for parity with hex (mix hex) here. Specifically, calling rebar3 hex publish assumes you want to publish both, we should check that docs exist before we attempt to publish anything, if they do not, bail and give the user a message.
We should also provide switches so inform publish to only publish the package or only publish the docs. --package and --docs.
The message I would expect in the case that we see no docs and bail would be something like what you have + If you really only want to publish the package without docs use the --package switch
Thoughts?
There was a problem hiding this comment.
I think that is a good idea, but I think the discussion around the details of implementing this new approach (i.e. publish = pkg+docs plus --package and --docs flags) deserves its own issue and separate PR. There might be many details that need to be discussed, for example, it would be nice to show the docs directory that will be used when listing the details on what is going to be published.
Since this PR address the bug in the current behaviour discussed in #180 I would like to get it merged to avoid the unexpected results we get today.
There was a problem hiding this comment.
To be clear, since I'm missing some context, we are running the edoc provider before publishing, right? If not we should have publish depending on or calling the edoc provider from rebar3 and then doing the publish.
If there aren't any docs even though it ran edoc then it should just continue on I would think, without erroring. Obviously will be an issue of someone overriding where edoc outputs shit, but whatever, can deal with that later.
There was a problem hiding this comment.
The publish command currently also publishes the docs by calling the internal function in rebar3_hex_docs:publish3. It is in this situation that the published docs are empty.
The docs command depends on {default, edoc}. This PR removes that dependency to allow users to rely on other tools to generate documentation.
There is more context in the related issue #177.
There was a problem hiding this comment.
Ah ok. Yea, I guess that function should call the edoc provider instead of having the provider use a dependency on it.
There was a problem hiding this comment.
Yeah, I'm cool with merging this now per what @jfacorro points out, i.e., it fixes a very confusing issue.
| @@ -1,6 +1,12 @@ | |||
| -module(rebar3_hex). | |||
There was a problem hiding this comment.
Whitespace only changes.
| @@ -1,18 +1,19 @@ | |||
| -module(rebar3_hex_client). | |||
There was a problem hiding this comment.
Whitespace only changes.
| @@ -1,15 +1,15 @@ | |||
| -module(rebar3_hex_config). | |||
There was a problem hiding this comment.
Mostly whitespace only changes.
| @@ -1,8 +1,7 @@ | |||
| -module(rebar3_hex_file). | |||
There was a problem hiding this comment.
Whitespace only changes.
| @@ -3,25 +3,28 @@ | |||
| %% @end | |||
There was a problem hiding this comment.
Whitespace only changes.
|
@starbelly @tsloughter Would it be possible to merge this? Or are there any changes that you think should be applied? |
There was a problem hiding this comment.
I plan on merging this today, I was waiting to see what @tsloughter thought overall, but I think he's inundated with rebar3 itself. I'm changing this back to requested changes as I need to test the bug myself and QA this branch. Will have an update later this evening.
Fixes #177