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

Documentation: Initial pandoc conversion #1586

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Nov 15, 2023

Add pandoc tools dependency to Squid.

Initial documentation file conversion to Markdown
see https://github.github.com/gfm/ for syntax.

Also, updated and polished the squid.8 content to
comply better with manual practices and Squid
current features / behaviours.

@yadij yadij marked this pull request as draft November 15, 2023 19:08
src/Makefile.am Outdated Show resolved Hide resolved
src/squid.md.in Outdated Show resolved Hide resolved
src/squid.md.in Outdated Show resolved Hide resolved
src/squid.md.in Outdated Show resolved Hide resolved
src/squid.md.in Outdated Show resolved Hide resolved
src/squid.md.in Outdated Show resolved Hide resolved
src/squid.md.in Outdated Show resolved Hide resolved
src/Makefile.am Outdated Show resolved Hide resolved
@yadij yadij requested a review from rousskov November 17, 2023 12:09
@yadij yadij marked this pull request as ready for review November 17, 2023 12:09
src/squid.md Show resolved Hide resolved
@yadij yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Nov 17, 2023
@yadij yadij requested a review from kinkie November 17, 2023 12:15
src/squid.md Outdated Show resolved Hide resolved
%.8: %.md
$(SUBSTITUTE) < $(srcdir)/`basename $<` | \
$(PANDOC) -t man -f commonmark -s -V title:`basename -s .md "$<"` -V section:8 -o $@

Copy link
Contributor

@rousskov rousskov Nov 17, 2023

Choose a reason for hiding this comment

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

$(PANDOC) ... -f commonmark

AFAICT, you have not disclosed the reasons for adopting CommonMark, so I may be missing some important caveats, but I think two competing (pandoc-supported) formats seem better suited for our purposes. They are listed here in second and third position:

  1. PR-proposed CommonMark: Minimal subset of popular flavors. Might make switching from pandoc to something else easier.
  2. gfm (Github-Flavored Markdown): A strict superset of CommonMark. May be usable for non-manual pages as well (e.g., future squid.conf directive documentation), allowing us to use a single Markdown flavor for all documentation written in Markdown. Supports tables and possibly footnotes (as well as strikethough text and task lists).
  3. markdown (Pandoc's Markdown): This is what pandoc itself is using by default, so it is very powerful. Supports metadata blocks, tables, and footnotes (as well as strikethough text, task lists, and a lot more).
  4. commonmark_x (CommonMark with many pandoc extensions): I do not think this is a viable candidate today due to this discussion. I am listing it here because it sounds attractive, and I thought it might be viable before doing this research.

I am currently torn between gfm and Pandoc's Markdown. Perhaps we should use the following decision-making logic:

  • If we can convince ourselves that we can (eventually) use gfm (Github-Flavored Markdown) for all Squid documentation written in Markdown, then we should use gfm because our GitHub-hosted web site (a.k.a. wiki) uses that or a superset of that format. Being able to use snippets from this repository for web site assembly can be very valuable!
  • Otherwise, we should use Pandoc's Markdown for producing well-formatted manual pages (and gfm for everything else).

Currently, the biggest missing/unknown piece of information for me here is whether gfm supports metadata blocks. For example, Pandoc can take manual page title from the document metadata section. Perhaps we can even use the same metadata format for squid.conf directive documentation metadata (i.e. current TYPE:, LOC:, etc. lines). If that metadata works with gfm (without making the rendered page uglier), then perhaps gfm is the best option for us.

Copy link
Contributor Author

@yadij yadij Nov 18, 2023

Choose a reason for hiding this comment

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

My reasons for selecting commonmark were:

  • @rousskov insisted on being formal right now without (either of us) having any idea of the amount of complexity our full set of documentation will require.
  • it is the portable subset of AFAICT all commonly used Markdown syntaxes. Allowing easy future changes of syntax if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for documenting your reasons for selecting CommonMark, addressing another change request. I have checked that this change request is not materially affected by the addition of that documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see any request for changes in your opening comment. Just a series of opinion statements about various alternatives and possibilities for future infrastructure.

If you do not want to demand a specific change to the format right now. Then I would like to defer that change to future PR and close off this initial upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see any request for changes in your opening comment.

I am against the format proposed in the current PR version -- commonmark. I have suggested two alternatives (and documented why they are better). I do not yet know which one is the best for moving forward (and there may be more candidates or even different formats for different kinds of files).

If you do not want to demand a specific change to the format right now. Then I would like to defer that change to future PR and close off this initial upgrade.

I would not describe my change request as "a demand for a specific change to the format". This PR (not me!) proposes a specific change to the existing format (of certain files). I support changing the exiting format (at least for some of the files), but not to the proposed one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then please state clearly and explicitly which Markdown format you are requiring everyone to use before I go and re-write the whole PR for a fourth time to one which you will only deem inadequate.

Copy link
Contributor

Choose a reason for hiding this comment

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

My proposal: replace commonmark with gfm in the pandoc invocation and leave everything else as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then please state clearly and explicitly which Markdown format you are requiring everyone to use

I cannot require any format in this context. As I said, I see two good format candidates and a usable decision making algorithm, but I do not have enough information to finalize that decision. IMHO, it is wrong to put the collection of missing information on me, but I will find the time to do it.

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Nov 20, 2023
@yadij yadij removed the S-waiting-for-author author action is expected (and usually required) label Nov 27, 2023
@yadij yadij requested a review from rousskov November 27, 2023 09:56
%.8: %.md
$(SUBSTITUTE) < $(srcdir)/`basename $<` | \
$(PANDOC) -t man -f commonmark -s -V title:`basename -s .md "$<"` -V section:8 -o $@

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see any request for changes in your opening comment.

I am against the format proposed in the current PR version -- commonmark. I have suggested two alternatives (and documented why they are better). I do not yet know which one is the best for moving forward (and there may be more candidates or even different formats for different kinds of files).

If you do not want to demand a specific change to the format right now. Then I would like to defer that change to future PR and close off this initial upgrade.

I would not describe my change request as "a demand for a specific change to the format". This PR (not me!) proposes a specific change to the existing format (of certain files). I support changing the exiting format (at least for some of the files), but not to the proposed one.

@kinkie
Copy link
Contributor

kinkie commented Dec 12, 2023

I would support GFM as a dialect; it has powerful backing and should be strict enough that we wouldn't end up in the situation we had with the wiki. I expect that all that's needed to adopt it is to change the argument to pandoc, so no need to redo translation work.

@yadij , can you agree to this?

@rousskov rousskov added the S-waiting-for-more-authors needs developer help label Dec 12, 2023
@yadij yadij requested a review from rousskov December 17, 2023 18:49
@yadij yadij removed the S-waiting-for-more-authors needs developer help label Dec 17, 2023
@yadij

This comment was marked as resolved.

@kinkie

This comment was marked as outdated.

@kinkie

This comment was marked as resolved.

@kinkie

This comment was marked as resolved.

@squid-anubis squid-anubis removed the M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels label Jan 9, 2024
@yadij yadij closed this Jan 22, 2024
@yadij yadij reopened this Jan 22, 2024
@kinkie
Copy link
Contributor

kinkie commented Jan 22, 2024

Currently, the biggest missing/unknown piece of information for me here is whether gfm supports metadata blocks

gfm supports - in fact, requires to be practically used - metadata in the form of Front Matter

@rousskov
Copy link
Contributor

Reviewer insists that we select a final and permanent Markdown syntax up-front and only use that one syntax forever, in all places Squid can or may now or in future utilize Markdown.

Nobody insists on (or even suggests) that.

src/squid.md Outdated Show resolved Hide resolved
@yadij yadij changed the title Docs: Initial pandoc conversion Documentation: Initial pandoc conversion Jan 25, 2024
@thesamesam

This comment was marked as resolved.

kinkie
kinkie previously approved these changes Feb 19, 2024
Copy link
Contributor

@kinkie kinkie left a comment

Choose a reason for hiding this comment

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

I tested this; it works. Thanks!

@yadij yadij added S-waiting-for-QA QA team action is needed (and usually required) M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Feb 25, 2024
@yadij yadij requested a review from rousskov November 10, 2024 10:28
@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-QA QA team action is needed (and usually required) M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels labels Nov 10, 2024
@yadij
Copy link
Contributor Author

yadij commented Nov 16, 2024

This PR has now passed the 10-days without objection criteria for merge.
Absent objections, I will clear the stale review and clear for merge in ~48hrs.

@rousskov
Copy link
Contributor

This PR has now passed the 10-days without objection criteria for merge.

It has not. This PR has a negative review from a core developer and, hence, cannot be merged.

Absent objections, I will clear the stale review and clear for merge in ~48hrs.

You do not have the rights to clear my negative review (regardless of whether it is "stale"), just like I do not have the right to clear yours. Please do not do that (with or without objections).

P.S. I am aware that you have recently requested my re-review and hope to re-review soon. I have not seen other recent comments/changes yet, so I cannot comment on them now.

Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

This is a partial review to make progress.

sudo apt-get --quiet=2 update
sudo apt-get --quiet=2 install libtool-bin
sudo apt-get --quiet=2 build-dep squid
sudo apt-get --quiet=2 install linuxdoc-tools libtool-bin pandoc
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to add linuxdoc-tools and pandoc to this functionality testing step in this PR? AFAICT, this step does not need those tools, even after this PR, because this PR does not require pandoc for "make install" to succeed.

Installing other optional dependencies here (i.e. build-dep squid and linuxdoc-tools) may be a good idea, but it outside this PR scope and may have complex/unexpected side effects. If we do not have to add linuxdoc-tools and pandoc for functionality tests, then we do not have to change anything in this step in this PR.

## squid.8: $(srcdir)/squid.8.in Makefile
## $(SUBSTITUTE) < $(srcdir)/squid.8.in > $@
.md.8:
$(SUBSTITUTE) < $(srcdir)/`basename $<` | \
Copy link
Contributor

Choose a reason for hiding this comment

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

.8 target (in this example and the actual use case) used to depend on Makefile. This change removes that dependency. Was that dependency wrong or unnecessary?


**squid** -n service -O command [-ds] [-\-foreground] [-l facility]

**squid** -z [-NS] [-dsX] [-\-foreground] [-l facility] [-f file] [-n service]
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR does not have to improve the old broken synopsis, but if we are going to do that, let's be consistent about listing options that require an argument. Option -d requires an argument but is shown as if it does not.

Comment on lines +15 to +20
**squid** -n service -i [-dsX] [-\-foreground] [-l facility]

**squid** -n service -r [-dsX] [-\-foreground] [-l facility]

**squid** -n service -O command [-ds] [-\-foreground] [-l facility]

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these three are Windows-specific variants. If they have to be listed here, please move them down, after all cross-platform variants that are going to be much more frequently used.

**squid** -n service -O command [-ds] [-\-foreground] [-l facility]

**squid** -z [-NS] [-dsX] [-\-foreground] [-l facility] [-f file] [-n service]

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this PR is trying to enumerate some or possibly all mutually exclusive ways to run "squid" in synopsis. IMO, there are too many of them and/or the enumeration principles are too complex -- the end result does not work well as a quick summary of how to start Squid. It would also be rather difficult to maintain as developers will forget to update some of these lines when adding options (among other potential problems).

I suggest simplifying the whole thing to just say something like:

# SYNOPSIS

**squid** [option]...

or

# SYNOPSIS

**squid** [options]

... and let individual option documentation to detail option compatibility, requirements, and other important caveats.

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants