Overrides of public API methods should be documented

[straight-shoota]

Overrides of public API methods are also public API and thus should be documented themselves.

An example is Array#index(object, offset : Int = 0) which overrides Indexable#index(object, offset : Int = 0) but is designated as :nodoc::

crystal/src/array.cr

Lines 2271 to 2272 in 5410318

	# :nodoc:
	def index(object, offset : Int = 0)

If you call index on an Array, the API docs would show that this method is implemented in Indexable, when it is in fact overridden in Array. When there's an error with calling that method, the compiler would show it resolved to Array#index, but this method can't be found in the public API docs.

This method overrides the previous implementation for a reason. The documentation can't really be identical, because it does something different.
So not only should it be documented, the differences to the original implementation should also be mentioned in the documentation.

[HertzDevil]

What about omitting the docs altogether, or :inherit: alone? And what about defs that are expected not to do anything different, e.g. overloads of def hash(hasher) within the same class hierarchy?

I made a patch that detects such :nodoc: overrides in the standard library. It finds 12 conflicting defs:

• Array#index overrides Indexable#index
• BigInt#digits overrides Int#digits
• Indexable#sample overrides Enumerable#sample
• IO::Memory#gets_to_end overrides IO#gets_to_end
• IO::Memory#peek overrides IO#peek
• IO::Memory#read_byte overrides IO#read_byte
• IO::Memory#skip_to_end overrides IO#skip_to_end
• Range#map overrides Enumerable#map
• Range#sample overrides Enumerable#sample
• Range#size overrides Enumerable#size
• Slice#index overrides Indexable#index
• StaticArray#index overrides Indexable#index

plus 14 more for defs from root types: (I'd argue these defs can implicitly inherit their docs from parents, like the hash example)

• Array, Enum, Log::Metadata#== override Reference#==
• JSON::Any, Log::Metadata::Value, Range, YAML::Any#to_s override Struct#to_s
• Array, Compress::Deflate::Reader, Compress::Deflate::Writer, JSON::Any, Log::Metadata::Value, Range, YAML::Any#inspect override Struct#inspect

The patch is here. There is the option of enforcing this in the style guide and the docs generator for any shards out there.

[straight-shoota]

I think omitting documentation (which implicitly inherits from the parent def) or an explicit :inherit: are fine. The former indicates that documentation is just missing. We're still missing documentation for lot's of methods, so that's acceptable. :inherit: makes an explicit choice that the documentation of the parent def applies to this one as well, so that should be perfectly fine.
The important aspect IMO is that these methods are not hidden via :nodoc:. Whether they have their own documentation or inherit implicitly or explicitly, is a decision for each individual case.

I like adding a warning to the doc generator 👍

[HertzDevil]

If #11089 is intended behaviour then one cannot assume that omitting doc comments implies the documentation is missing, because the generated docs are visibly different between no comments at all and a bare :inherit:.