erlef / rebar3_hex

Rebar3 Hex library
Apache License 2.0
101 stars 49 forks source link

Revisit publishing docs without any configuration #213

Closed starbelly closed 2 years ago

starbelly commented 3 years ago

Per #185, #210 , and #211 we now require a doc provider to be specified to auto-generate docs, we offer options to publish a package using the --without-docs switch on the publish task and we offer a --dry-run option on the docs task so a user can check generated docs before publishing.

However, we still have this support for just publishing docs if a doc directory exists and as soon as #212 is in, only when it contains an index.html file, at least. This still feels haphazard. I think we should probably only publish docs by default on the publish task if a doc provider is configured, and we should only publish docs on the docs task without a doc provider in place task if a switch is given. I don't like the implicitness of saying "Well, we see a docs dir, and it has an index.html file, so I guess this is fine". As to what the switch would be called, I'm not sure. Perhaps it could specify a docs dir to use so that the operation is very explicit for the case.

If we are in agreement about this, I'll get in a quick PR, hopefully tomorrow and before we merge the current PR I have open to rebar3.org repo.

ferd commented 3 years ago

yeah I'm fine with going safer that way. If we want to nudge people in the direction of documentation, maybe going with something like {docs, [...]} not configured, documentation is not being published or something could still encourage people to write it up.

eproxus commented 3 years ago

Small request: never publish docs even if an index.html file is present. Only do this if there’s a static file doc source configured in rebar.config. I think the risk of publishing stale or wrong documentation is too large for it to be a sensible default.

starbelly commented 3 years ago

@eproxus that's what the issue is about. If you don't want to use edoc, this issue proposes a --docs-path option on the docs and possibly the publish task to make the operation explicit to negate causing issues for others who do in fact use edoc.

eproxus commented 3 years ago

@starbelly Perfect. Thought there was going to be a heuristic based on index.html.

starbelly commented 3 years ago

@eproxus That will still be checked off course, as a sanity check, but it won't be used as a heuristic as you suggest. If you're good on this, I'd like to close #205.

paulo-ferraz-oliveira commented 3 years ago
  1. if I understand the above correctly, this would make it a behaviour-breaking change, and not just new options, right?

I mean, I'm used to just rebar3 hex publish ... and that's it, along go the docs. As Fred stated, in that case, I'd probably add a note, at the end of the "publish" procedure stating what happened ("Docs have (not) been published"). Rationale: I know plugging publication to e.g. GHA is probably not a good practice, and I also understand that relying on master is probably not a good practice either, but unfortunately both exist and any change that is not "visible" will break current behaviour.

  1. IIRC, Fred mentioned something about Elixir having lots of doc and Erlang not; changing the behaviour to only publish docs when a provider is set will not help having more Erlang docs out there, right?

  2. Also, @starbelly, I feel we're mixing two things here, since you mention "if a doc provider is configured" (generation+publication). What if I'm doing my docs completely outside any "normal" tool and don't want to plug it into rebar3 (plugins, or whatever) but still would like to publish, if doc/index.html is present? What provider would I use?

  3. @eproxus, "if there’s a static file doc source configured in rebar.config" will mean that every time you don't want to publish docs you have to change rebar.config before it, right?

It doesn't feel natural. At the same time, just a thought, did we consider looking at timestamps to understand if we should publish docs or not (if the worry is publishing stale documentation, if it's been refreshed it shouldn't be stale, anymore).

starbelly commented 3 years ago

if I understand the above correctly, this would make it a behaviour-breaking change, and not just new options, right?

Yeah, I think a new major version makes sense, perhaps, sense the behavior will change a good bit, but also, we already broke it. When the behavior of implicitly uploading a docs directory and removing auto-generation of edoc was removed, we broke our contract. In some ways, like @eproxus alluded to this can be considered a regression. That said, I don't have a problem doing a major ver bump, but also we won't be supporting 6.x ofc.

IIRC, Fred mentioned something about Elixir having lots of doc and Erlang not; changing the behaviour to only publish docs when a provider is set will not help having more Erlang docs out there, right?

This is how mix hex works. Docs don't get published unless ex_doc is configured and iirc if you don't have ex_doc as part of the setup you get a fatal on their publish task, but can still use package task to publish with out docs.

Also, @starbelly, I feel we're mixing two things here, since you mention "if a doc provider is configured" (generation+publication). What if I'm doing my docs completely outside any "normal" tool and don't want to plug it into rebar3 (plugins, or whatever) but still would like to publish, if doc/index.html is present? What provider would I use?

That is precisely what the issue covers, if you want to use another tool, you are free to do so, but it's not good to make things "unsafe" for the rest of our users. Thus, you add an option that makes pointing to a doc directory to instruct to "Yes, I really want to just publish the contents of this dir" explicit. That path is not the norm and has caused issues for users on the common path.

@eproxus, "if there’s a static file doc source configured in rebar.config" will mean that every time you don't want to publish docs you have to change rebar.config before it, right?

No, if you don't want to publish docs, and as of right now there is an option on publish you can invoke so that you don't publish docs. Once again, this entire issue is around getting rid of implicit behavior around publishing docs.

starbelly commented 3 years ago

Let me see if I can demonstrate all this with some code snippets and cases :

In the case you have no doc provider configured and you attempt to run publish there are one of two ways we can go :

publish task

$ rebar3 hex publish 
==> Error: You have no docs provider configured, if you really want to publish without docs use rebar3 hex package to publish

or

$ rebar3 hex publish 
==> Error: You have no docs provider configured, if you really want to publish without use --without-docs option

or

$ rebar3 hex publish 
==> Warning: You have no docs provider configured, docs will not be published, use the docs task with --docs-dir to publish docs devoid of a provider. 

doc task

$ rebar3 hex docs 
==> Warning: You have no docs provider configured, docs will not be published, use --docs-dir to publish docs devoid of a provider. 

And FWIW the reason there are docs on probably every elxiir lib on hex is because they get sent down the right path and there are no surprises. IMO of course.

starbelly commented 3 years ago

And while I think about it, @ferd was right, there should be --docs and --package options on the publish task, once again to be more in line with mix hex. I originally stated I wanted to do that, but I strayed, but after reflection, that's the way to go. Thus, a major version bump is indeed in order IMO.

Edit:

FWIW https://github.com/hexpm/hex/blob/6f4445583cb349d4bd6a9eb03cf187a330397a65/lib/mix/tasks/hex.publish.ex#L183

As you can see, the task will attempt to build docs, if ex_doc is not setup, it will crash. mix hex should probably gracefully handle this, but I next to never see this come up because those in the elixir community usually and AFAIK already have ex_doc added to their project.

Edit:

It appears they do gracefully handle this now per : https://github.com/hexpm/hex/blob/6f4445583cb349d4bd6a9eb03cf187a330397a65/lib/mix/tasks/hex.publish.ex#L223

Edit:

Final edit. I have made up my mind on this. I want to follow the behavior of mix hex to a t. Of course, it's not only up to me, I need to hear from @tsloughter. I do think we should bump to version 7.

ferd commented 3 years ago

Doing the same as Hex with a major version bump sounds good to me.

starbelly commented 3 years ago

I'm going to make a version 7 issue and enumerate all the things we need to change to get more aligned, that would be considered breaking changes. Would hate to do a major ver bump, and then another, and another, etc. I don't believe it's much as that's what we've been working on anyway. We will always have a few unique pieces, such as supporting multiple repositories (i.e., not hex.pm repos), what the issue alludes to (i.e., support for publishing docs outside of the common path), but in general it should be limited to just a few things.

starbelly commented 2 years ago

Closed via #240