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

cmd: export CaddyVersion(), Commands() #4316

Merged
merged 2 commits into from Sep 2, 2021
Merged

Conversation

pymnh
Copy link
Contributor

@pymnh pymnh commented Aug 31, 2021

Initial draft adressing caddyserver/dist#29

Because it's not yet clear where to put this package, I have put all code in one single file.

As considered by @mholt, I could probably rewrite this as a Caddy module. However I think this would require exporting the "commands" variable from caddycmd, I am not sure if that could be a problem. Please let me know what you think

@CLAassistant
Copy link

CLAassistant commented Aug 31, 2021

CLA assistant check
All committers have signed the CLA.

@pymnh pymnh mentioned this pull request Aug 31, 2021
@pymnh pymnh changed the title [Draft] generate manpages Command for generating manpages Aug 31, 2021
@francislavoie francislavoie added this to the v2.5.0 milestone Aug 31, 2021
@francislavoie
Copy link
Member

Do you mind pasting the output of this new command in a Gist or something? Hard to envision what the template's output would look like without running it

@mholt
Copy link
Member

mholt commented Aug 31, 2021

Yeah, I think an external module would be best for this.

Instead of exporting the commands variable, how about a getter function instead?

@pymnh
Copy link
Contributor Author

pymnh commented Aug 31, 2021

Of course... 😅

So, these are the files I generated for the Debian package. Unfortunately the roff markup language is still rather opaque...
I rendered some to html pages which resembles the final manual page shown in a terminal emulator much closer, see here and here

@pymnh
Copy link
Contributor Author

pymnh commented Aug 31, 2021

Yeah, I think an external module would be best for this.

Instead of exporting the commands variable, how about a getter function instead?

Sounds good. I will look into that

@francislavoie
Copy link
Member

Nice! Thanks for the preview!

We'll probably want to commit these to the /dist repo as well for the Fedora COPR and our Cloudsmith apt repos to inherit them.

We can probably also add a step in CI to automatically regenerate them for /dist as well on release.

@pymnh pymnh marked this pull request as ready for review August 31, 2021 23:19
@pymnh
Copy link
Contributor Author

pymnh commented Aug 31, 2021

Did some clean up and made it a module

Some notes:

I have added Commands() to caddycmd. Is that what you were thinking of? I knew Getters as a function to a Type, so I was a bit unsure if I understood right.

I have also exported caddyVersion() so that this information can be included in the manpage. Is that alright?

I also looked at other source files and tried to be comparably explicit with comments throughout the file (maybe even too explicit?)

Please let me know what you think

@mholt
Copy link
Member

mholt commented Sep 1, 2021

Yep, exporting those two functions like that works, I'd say.

Nice job. I think we should put this module in a new repo. Maybe caddyserver/manpages - what do you think?

The changes to Caddy that export the functions can stay or be opened in a new PR.

@mholt mholt added the do not merge ⛔ Not ready yet! label Sep 1, 2021
@francislavoie
Copy link
Member

francislavoie commented Sep 1, 2021

It doesn't necessarily need to be in a new repo, we could put it in /dist maybe? github.com/caddyserver/dist/modules/manpages?

@mholt
Copy link
Member

mholt commented Sep 1, 2021

Sure, that's an option too. Or just dist/manpages.

@pymnh pymnh changed the title Command for generating manpages cmd: export CaddyVersion(), Commands() Sep 1, 2021
@pymnh
Copy link
Contributor Author

pymnh commented Sep 1, 2021

opened PR here: caddyserver/dist#73

francislavoie
francislavoie previously approved these changes Sep 1, 2021
@francislavoie francislavoie added under review 🧐 Review is pending before merging and removed do not merge ⛔ Not ready yet! labels Sep 1, 2021
@pymnh
Copy link
Contributor Author

pymnh commented Sep 1, 2021

forgot to go fmt 😬
this should do

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.

LGTM. Thanks for the simple change, looking forward to some legit man pages :)

@mholt mholt merged commit 9f6393c into caddyserver:master Sep 2, 2021
@mholt mholt modified the milestones: v2.5.0, v2.4.5 Sep 2, 2021
@mholt mholt removed the under review 🧐 Review is pending before merging label Sep 2, 2021
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.

None yet

4 participants