-
Notifications
You must be signed in to change notification settings - Fork 529
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
Bug 5093: List http_port params that https_port/ftp_port lack #1977
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on improving documentation. Unfortunately, this PR has many small problems. I can fix them myself later, but if you do not want to wait, here is a brief laundry list:
- Please proofread duplicated changes for typos.
- Please adjust PR title prefix for consistency with other recent bug fixes. Adding ftp_port to the title and making the title more specific to PR changes is probably worth doing as well.
- If you are sure that all unlisted options are supported, please disclose that in the PR description or GitHub comment. Otherwise, please rephrase "not supported" paragraphs to avoid an implication that the lists are exhaustive (i.e. that all unlisted options are supported).
- Please fix/change indentation of new paragraphs to match indentation of the paragraph above them. IIRC, we have to use a single tab character here.
- For each port, please integrate the new paragraph with the old one (above the new one) that talks about very similar things.
- I would completely remove a stale (or somewhat inconsistent-with-PR-changes) and, IMO, unnecessary PR description.
Fixed
I'm out of ideas on how to do it in 56 characters :
Corrected
Hopefully the new wording is an improvement
Tried updating it, feeel free to remove it altogether. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please adjust PR title prefix for consistency with other recent bug fixes. Adding ftp_port to the title and making the title more specific to PR changes is probably worth doing as well.
I'm out of ideas on how to do it in 56 characters : Hopefully it's better now
I fixed the prefix and polished the rest of the title a bit. Updated PR description explains that this change does not list all unsupported parameters. Please adjust further as needed.
FWIW, Anubis-enforced commit title length limit is not 56 characters.
- For each port, please integrate the new paragraph with the old one (above the new one) that talks about very similar things.
Hopefully the new wording is an improvement
Yes, it is. The wording, formatting, and consistency of those two updated paragraphs is still quite poor IMO, but I do not have the time to improve that text (especially given our unfortunate cf.data.pre indentation requirements!), and am not going to block this PR because of those secondary problems. It is better to add that text than to add nothing.
To avoid documentation duplication, current https_port and ftp_port
directive descriptions reference http_port directive instead of
detailing their own supported parameters. For https_port, this solution
creates a false impression that the directive supports all http_port
options. Our ftp_port documentation is better but still leaves the
reader guessing which options are actually supported.
This change starts enumerating http_port configuration parameters that
ftp_port and https_port directives do not support. Eventually, Squid
should reject configurations with unsupported listening port options.