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

Format markdown comments in modules.caddyhttp.App #5960

Closed
wants to merge 1 commit into from
Closed

Format markdown comments in modules.caddyhttp.App #5960

wants to merge 1 commit into from

Conversation

sword-jin
Copy link

image

Format these comments.

@CLAassistant
Copy link

CLAassistant commented Dec 1, 2023

CLA assistant check
All committers have signed the CLA.

@francislavoie
Copy link
Member

We're formatting it so it works here:

https://caddyserver.com/docs/json/apps/http/#docs

There's a risk that it might break those docs, so we'll need to check on that before merging this.

@sword-jin
Copy link
Author

We're formatting it so it works here:

https://caddyserver.com/docs/json/apps/http/#docs

There's a risk that it might break those docs, so we'll need to check on that before merging this.

Thank you, maybe we need to fix it if it's not compatible, I think some parts of developers are used to read docs on godoc.

@francislavoie
Copy link
Member

Yeah definitely, I agree. Just want to make sure it looks right for both, if possible. But we prioritize our docs first.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Woah. Why does that help? I'm not sure I understand 😇

@sword-jin
Copy link
Author

By the way, how to compile https://github.com/caddyserver/moduledoc, I didn't find any docs about it. I want to compile on my local environment so I can check it's comparable with our doc system

@mholt
Copy link
Member

mholt commented Dec 1, 2023

Huh. When I wrote my review none of the other comments showed up for me (on my phone). I really need to stop trusting the GitHub mobile app.

Yeah, the priority is that it renders properly as markdown on our site.

In order to test the JSON docs locally though you need a DB and a portal repo which isn't currently published. So now that I've seen the other comments, I'll clarify that I haven't verified that yet myself.

What does the godoc look like after this change?

My fear is that this change will cause these docs to go into a big code block instead of being interpreted as markdown.

@sword-jin
Copy link
Author

image
Like that
@mholt

@mholt
Copy link
Member

mholt commented Dec 4, 2023

Thanks, that does look a lot better on godocs; it also confirms my suspicions that I think it will cause the docs on our Caddy website to look like that -- one big code block with literal markdown in it. I don't know a way to make it look nice in both places. We might have to choose.

I think most people are reading the docs for templates on our website (which renders Markdown). What's your use case for reading it on the godoc instead of on our website?

@mholt mholt added under review 🧐 Review is pending before merging documentation 📚 Improvements or additions to documentation labels Dec 4, 2023
@sword-jin
Copy link
Author

@mholt
I am used to go though all the API and types on the go doc before I jump into the code. I think maybe other developers have same habit like me,

@mholt
Copy link
Member

mholt commented Dec 18, 2023

That's interesting.

I wish we could make both look good, but unfortunately I don't know a way to do that.

I think our first priority needs to make sure that our official docs on our website are legible, especially given that the table shown there isn't intended for Go programmers as much as Caddy users, which likely won't be in the godoc, I'm afraid.

Thanks for the thoughtful change, I just wish we could find a way that wouldn't break our current docs!

Closing, but if a viable solution can be found, we can reopen.

@mholt mholt closed this Dec 18, 2023
@mholt mholt added declined 🚫 Not a fit for this project and removed under review 🧐 Review is pending before merging labels Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
declined 🚫 Not a fit for this project documentation 📚 Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants