[DOC] Revision for XML::Node.add_child #3431
Conversation
|
@flavorjones, much that follows will depend on this PR (b/c the example code will be cited). |
lib/nokogiri/xml/node.rb
Outdated
| # | ||
| # Also see related method +<<+. | ||
| # :call-seq: | ||
| # add_child(object) -> object |
There was a problem hiding this comment.
I've been gradually introducing a documentation convention to give a brief overview of arguments and argument types, which I'd like to ask you to use (or improve upon).
Good examples of this convention are in this file, and applying it here might look like:
[Parameters]
- +object+ (String, Node, NodeSet) The Node or Nodes to be added as children of this node. A String will be parsed as a fragment and the resulting NodeSet will be added as children.
followed by the prose explanation and examples below. WDYT?
There was a problem hiding this comment.
What you're suggesting fits well with my own personal philosophy. (That's how I wrote in various DEC programmer's reference manuals back at the Dawn of Time.)
However, I think you should consider the Ruby Documentation Guide, first drafted by @jeremyevans and often discussed and augmented since. It represents consensus in the Ruby community, which I think has value in itself. But it also gives a consistent "Feel" to the Ruby documentation that readers might appreciate also finding in the gems' documentation.
But of course, "your call."
There was a problem hiding this comment.
Do you mean this section in the Documentation Guide? I don't find it specifies much of anything beyond "use a labeled list", but to be clear I'm fine with that if that's what you want to go with.
The general principal is: give a brief summary of arguments and their expected types. I'm not finicky about the particulars of the style we use to present it.
There was a problem hiding this comment.
Also, the "if necessary" bit in that section makes me groan a bit. I get it, we don't need to over-specify things for simple methods with simple arguments.
But all of these Node methods in particular take complex arguments (could be a Node, could be a NodeSet, could be a String that is cast as a NodeSet via fragment parsing) and so, yes, I think for those kinds of methods, documenting the arguments is necessary. WDYT?
|
@flavorjones, as guidance going forward: You prefer |
|
@BurdetteLamar Oh! I don't actually have any feelings here. I suppose if it's XML then |
|
Marked as draft, while I make a little more consistent. |
There was a problem hiding this comment.
This looks good to me, adding the examples is an obvious improvement over what was there.
I would still like to have a brief summary of the types that the input argument can be. Again, I don't feel strongly about the style used to present it, but I do feel strongly that the types and behaviors are complex enough that they justify being summarized very briefly for users who are looking for API reference material and not an explanation. We need to provide both, I think.
lib/nokogiri/xml/node.rb
Outdated
| # | ||
| # Argument +object+ is one of: | ||
| # | ||
| # - A Nokogiri::XML::Node (of any kind). |
There was a problem hiding this comment.
"any kind" isn't technically true, it must be a Node but also cannot be a Document or an Attr.
There are also additional constraints applied based on the type of self, which probably aren't worth going into detail about here.
Maybe we can just change drop the "(of any kind)".
lib/nokogiri/xml/node.rb
Outdated
| # #(Element:0x5992dc { name = "bar", children = [ #(Text "BAR")] }), | ||
| # #(Element:0x5994bc { name = "baz", children = [ #(Text "BAZ")] })] | ||
| # | ||
| # Raises ArgumentError if +object+ is not a node, a nodeset, or a string. |
There was a problem hiding this comment.
In general, I think information about return types, exceptions, and blocks should be the top of the docstring near the information about argument types.
lib/nokogiri/xml/node.rb
Outdated
| # | ||
| # Also see related method +<<+. | ||
| # :call-seq: | ||
| # add_child(object) -> node or nodeset |
There was a problem hiding this comment.
When possible, I'd like to use the proper constant name when we reference classes.
| # add_child(object) -> node or nodeset | |
| # add_child(object) -> Node or NodeSet |
There was a problem hiding this comment.
Again, different from what's seen in Ruby core and standard libraries. And again, I'll do as you say.
lib/nokogiri/xml/node.rb
Outdated
| # | ||
| # - A Nokogiri::XML::Node (of any kind). | ||
| # - A Nokogiri::XML::NodeSet. | ||
| # - A string. |
There was a problem hiding this comment.
| # - A string. | |
| # - A String. |
There was a problem hiding this comment.
Same as above.
lib/nokogiri/xml/node.rb
Outdated
| # | ||
| # Appends child nodes to +self+. | ||
| # | ||
| # Argument +object+ is one of: |
There was a problem hiding this comment.
Ugh, reading the previous documentation, I'm now realizing that object could also be a DocumentFragment
|
As above, I'll do as you say. Thanks for taking time on this PR, as it establishes what we're trying to do. |
|
Converting to draft (again), while I mull things over. |
|
@flavorjones, I have greatly modified this PR. It can (I hope) serve as a model for what's to come. A few points:
|
|
@flavorjones, let's wait on this. I've only now discovered |
No description provided.