-
Notifications
You must be signed in to change notification settings - Fork 29
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
Nodocs and remove warning #70
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Nov 21, 2022
Merged
nevans
force-pushed
the
nodocs-and-remove-warning
branch
from
November 22, 2022 14:49
d7bf065
to
dd130b4
Compare
Switch to the official RFC citation format, as reported at each RFCs txt citation URL, e.g: https://www.rfc-editor.org/refs/ref9051.txt. Removed "EXT-" from any citations that were still using it, because that is how modern IMAP RFCs cite their references. Add links to relevant specifications for response data structs, without resorting to the full citation syntax. ---- This is closer to what the format had already been before I switched it up the last time (9cd562a). I believe the earlier format was simply copy/pasted from the references section of whichever RFC was being used at the time (e.g. 2060 or 3501). But the RFC citation format has changed since then, too. I'd previously changed it to be more "readable". But since then I've added links throughout the documentation of methods and data types, only occasionally needing a list of "see also" links. That approach works and satisfies my readability concerns. So our references list should just be a standard list of citations and it would be better to use the official citation format. To consider: should we move the references list to a REFERENCES.md file?
nevans
force-pushed
the
nodocs-and-remove-warning
branch
from
November 22, 2022 14:55
dd130b4
to
153adfa
Compare
@shugo: the reference to |
shugo
approved these changes
Nov 23, 2022
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.
It looks fine!
"openssl" has been a default gem for a long time. Although some installations may still disable it, this is very rare and not important enough to document here any more. Looking at "net-http" and "net-smtp", neither of them still document the scenario where "openssl" is missing. Although it carefully avoids an implicit dependency, "net-http" doesn't try to catch any LoadError when an SSL connection is explicitly used.
RESPONSE_ERRORS is used internally and should probably be private_const. client_thread should be removed (or deprecated).
nevans
force-pushed
the
nodocs-and-remove-warning
branch
from
November 23, 2022 13:29
153adfa
to
23c2a4f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I propose that:
We should deprecate
client_thread
. This PR only marks it with:nodoc:
.We should ark
RESPONSE_ERRORS
with:nodoc:
. We don't need to break it if people are using it, but it's an internal implementation detail, in my opinion.Don't need to document the
openssl extensions must be installed
scenario. We can still try to support that use-case, but "openssl" has been a default gem for a long time.Although some installations may still disable it, this is very rare and not important enough to document here any more. Looking at
net-http
andnet-smtp
, neither of them still document the scenario where "openssl" is missing. Although it carefully avoids an implicit dependency, "net-http" doesn't try to catch any LoadError when an SSL connection is explicitly used.