Skip to content
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

Move to ex_doc -style documentation #112

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Jul 27, 2023

Note: Depends on the CI file updates from #114, so shall be rebased and updated (if required) after that one. Closes #121.

What this pull request is

  • moving from a EDoc/.md -style to an ExDoc/Hexdocs.pm -style, plus
  • making sure rebar3_ex_doc runs smoothly on this, via rebar3 ex_doc, thus
  • modernising the documentation

What this pull request is not

  • an exhaustive attempt at documenting Elli's internals (or even changing the user-facing pages significantly)

Bonus

  • .md files were linted and adjusted according to results

Notes

  • I completely removed the doc folder, since I expect the documentation to be hosted next to hexdocs.pm, but lemme know if this was a good call
  • I call elli to the modules, but Elli to the project consistently (this wasn't 100% done)

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jul 27, 2023

I did this change without first checking if the Issues or Pull requests mentioned it (glad they didn't, so my time wasn't wasted 😄), but now see it might even fit well with #102, that mentions:

This was referenced Jul 27, 2023
@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the feature/ex_doc branch 2 times, most recently from 8ed9115 to 0e461a6 Compare July 27, 2023 20:59
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.08%. Comparing base (05e24b1) to head (828b981).
Report is 34 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   76.30%   76.08%   -0.23%     
==========================================
  Files          12       12              
  Lines         764      740      -24     
==========================================
- Hits          583      563      -20     
+ Misses        181      177       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulo-ferraz-oliveira
Copy link
Contributor Author

CI failing for OTP 26. I'll try to replicate locally.

@paulo-ferraz-oliveira
Copy link
Contributor Author

Removed OTP 26 from scope, since there're SSL -related issues to solve. Should be moved to a new PR, I guess...

@paulo-ferraz-oliveira
Copy link
Contributor Author

Is anything lacking for this one to get merged? I can then rebase and update the other ones.

@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the feature/ex_doc branch 2 times, most recently from 5db0d64 to 6b0a4d1 Compare August 2, 2023 21:10
@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as draft October 10, 2023 15:20
@paulo-ferraz-oliveira
Copy link
Contributor Author

Moved to draft. These don't need to be reviewed before #114, after which I'll rebase.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@tsloughter

@tsloughter
Copy link
Member

My only concern is people likely got used to having the docs there in the github repo. But moving to exdoc is def needed and the future, seeing as OTP itself has done so. Perfect world we could have both.

Resolve the conflicts and I'll merge.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jun 30, 2024

I don't mind pushing the docs to the repo, if that makes sense. I'll test it in this pull request, since I don't know how it'd be rendered/accessible.

Edit: I don't believe I've removed it, so am wondering where "in the github repo" you could see this? Rendered on the source code directly?

It'll be online at hexdocs.pm
Also moves @doc to before -spec, where it's most commonly found
Also moves @hidden > @Private
Also moves @equiv to "Same as"
Also moves @link to `, since ExDoc does auto-reference
Also moves @see to `, for the same reason as @link
(and reference it from README.md)
@paulo-ferraz-oliveira
Copy link
Contributor Author

@tsloughter, tests are passing. Lemme know if you want me to change something, or we can also create issues and look at it later, if it's not blocking. My goal would be, after this, to get tests running on 26 and 27, and then request a release.

@tsloughter
Copy link
Member

@paulo-ferraz-oliveira sorry, wasn't clear. I meant just if it could be markdown. Since it can't be generated to markdown we can remove it. After you remove it I'll merge this.

@paulo-ferraz-oliveira
Copy link
Contributor Author

I meant just if it could be markdown.

What I thought 😄 Alas, it can't. But should it be possible? I mean, we write Markdown in the doc.s, then it gets converted to HTML, so surely something can be done (?)

@paulo-ferraz-oliveira
Copy link
Contributor Author

@tsloughter, I think now we're ready 😄

Copy link
Member

@tsloughter tsloughter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope everyone is good with this move and the removal of the markdown :). I think it is the right call, esp given OTP has even moved to ex_doc.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@tsloughter, shall we tag somebody to do extra validation on this, or is it Ok?

Copy link
Member

@yurrriq yurrriq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just left a couple comments

%% The result is either a `byte_range_set()' or the atom `parse_error'.
%% Use {@link elli_util:normalize_range/2} to get a validated, normalized range.
%% The result is either a `[http_range()]' or the atom `parse_error'.
%% Use `elli_util:normalize_range/2' to get a validated, normalized range.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will ex_doc make a link here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, yeah; it's pretty good at that. But lemme try it out, locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@@ -745,7 +745,7 @@ split_path(Path) ->
P =/= <<>>].

%% @doc Split the URL arguments into a proplist.
%% Lifted from `cowboy_http:x_www_form_urlencoded/2'.
%% Lifted from cowboy_http:x_www_form_urlencoded/2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some particular reason to drop the ` and ' here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No special reason. Lemme re-check the links.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely because it's a link to an external project, so it wouldn't know where to point, but I can check...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you mean you want to keep the notation even if there's no link, which seems to be the case.

ex_doc is already smart enough to turn the call
into a link to the Erlang docs
@paulo-ferraz-oliveira
Copy link
Contributor Author

Push two minor changes (latest commits) based on review and self-review. I believe it's possible to publish the docs alone (to test them out?) even though I've never done it before.

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.

docs: move to ex_doc
4 participants