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

Add a manpage to Zellij #455

Merged
merged 12 commits into from
May 5, 2021
Merged

Add a manpage to Zellij #455

merged 12 commits into from
May 5, 2021

Conversation

Adhalianna
Copy link
Contributor

After a shock of failing at man zellij I've decided to work on that as my first OSS contribution.

I have added a new dev-dependency mandown but I am not sure if that is the way to go as it is not installed automatically this way before invoking cargo make manpage.

I believe mandown (command-line utility in this case, not the library) will make updating the manpage easier. I used it to create the manpage avaliable under assets/man/zellij.1 and added two new tasks to Makefile.toml, one runs mandown to generate manpage from a file at docs/MANPAGE.md the other just installs ready manpage from assets.

@Adhalianna
Copy link
Contributor Author

I've decided to remove mandown from dependencies and removed the task installing manpage (this should probably be a task of a package manager). Still I think it is convenient to generate manpage from a markdown file with mandown and to keep that markdown file in docs. Instead I've added a line in CONTRIBUTING.md explaining that one should have mandown to run cargo make manpage. Since updating manpage is not a developer's daily task nor every developer has to do this, having to install mandown manually should not be that much of issue?
(I'm probably just panicking over first public contribution)

@Adhalianna Adhalianna marked this pull request as ready for review May 5, 2021 10:10
@a-kenji
Copy link
Contributor

a-kenji commented May 5, 2021

Thank you, this is great!
I personally am OK with the dev dependency.

I've decided to work on that as my first OSS contribution.

👍

We have been trying to get away from including generated assets - if we can,
do you think it would be feasible to only include the markdown version of the manpage, and do the conversion at publish/build time?

Or would that have significant drawbacks in your eyes?

@a-kenji
Copy link
Contributor

a-kenji commented May 5, 2021

I've decided to remove mandown from dependencies and removed the task installing manpage (this should probably be a task of a package manager).

Yes, that is a good point! I am not sure what is easier for most distributions and to what extent they ingest the Cargo.toml and Makefile.toml, but I think sticking to one thing for now and hoping for feedback from maintainers is a good way to go for now.

Still I think it is convenient to generate manpage from a markdown file with mandown and to keep that markdown file in docs. Instead I've added a line in CONTRIBUTING.md explaining that one should have mandown to run cargo make manpage.

I agree. I doubt we would be too happy writing the manpages from scratch.
Since it is no longer a dev dependency I think I am more OK with this being an asset, maybe more people will have some input here. Maybe it is something that CI can generate reliably for us, or while pushing to crates.

Thank you, this is very valuable!
From my standpoint this should be good to merge, I'll wait a bit for more input.

cc @TheLostLambda

@Adhalianna
Copy link
Contributor Author

Adhalianna commented May 5, 2021

cargo make manpage is still there to run mandown and could be included to the release build flow, the only issue I have is that it needs to be downloaded manually. I am willing to play with it some more and see if there are some better ways of integrating it so that it is installed automatically. One that I can think of right now is using mandown as a library and running it from build.rs but that makes build files more complicated which might make it harder to maintain in the future

@TheLostLambda
Copy link
Member

TheLostLambda commented May 5, 2021

@Adhalianna I believe that the ci-build-release flow already does a cargo install of cargo-cross, so I think it's quite reasonable to add mandown to that CI flow as well. Similar to @a-kenji 's earlier comment, I'm not a super massive fan of keeping the built manpage asset in the repository. Is there a reason we need that stored there? (Just makes for non-ideal diffs when you version control auto-generated files).

As I understand, the manpage doesn't actually do anything on crates.io but is just around for those doing distribution packaging? If so, it might be nice to generate it during CI and have it as a release asset.

@Adhalianna
Copy link
Contributor Author

Then the generated asset can be safely removed

@Adhalianna
Copy link
Contributor Author

Now ci-build-release will install mandown. As for normal install (cargo make install), it will run cargo install mandown which should result in installing mandown only if it's not already there. Package maintainers should probably mark then mandown as a dependency in case of packages built from source.

Still I am much more of a fan of leaving the asset. It would keep the install from sources smaller. Only those interested in editing the manpage would have to install mandown.

@a-kenji
Copy link
Contributor

a-kenji commented May 5, 2021

Thank you for all the work,
the manpage looks great!

Now ci-build-release will install mandown. As for normal install (cargo make install), it will run cargo install mandown which should result in installing mandown only if it's not already there. Package maintainers should probably mark then mandown as a dependency in case of packages built from source.

Ah, so the conversion is done again? Does it make sense to somehow get the assets that is released to crates.io from crates.io through the install? Or does that just unnecessarily complicate things? Or is that not feasible?

As I understand, the manpage doesn't actually do anything on crates.io but is just around for those doing distribution packaging? If so, it might be nice to generate it during CI and have it as a release asset.

Still I am much more of a fan of leaving the asset. It would keep the install from sources smaller. Only those interested in editing the manpage would have to install mandown.

Thanks for raising this point! This is a good idea. If we really need to install manpages and build them on every cargo install, maybe it does make sense versioning the asset too.

@a-kenji
Copy link
Contributor

a-kenji commented May 5, 2021

@Adhalianna
Thank you for all the great work!
And congratulations on your first OSS contribution,
I hope it was pleasant.
I will merge it as is for now. If you want to discuss further and see
merit in including it as an asset please open an issue and link to this pr,
so we can discuss it, maybe even with more people involved!

@a-kenji a-kenji merged commit bee1082 into zellij-org:main May 5, 2021
a-kenji added a commit that referenced this pull request May 5, 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.

3 participants