crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.45k stars 1.62k forks source link

Explicit classification of constructors for API docs #10617

Open WhereIsX opened 3 years ago

WhereIsX commented 3 years ago

In the API for File, the constructor lists: File.new, File.open

Meanwhile, the overloaded version of File.open(... &) that accepts a block is excluded from the constructor section and 2/3s of the way down the page. I feel that overloads should stick together as much as possible, especially when the examples in the Overview section at the top of the page includes the use of File.open(... &)

Specifically, I think it would be more beginner-friendly if File.open(... &) were included in the constructor section.

HertzDevil commented 3 years ago

It is not a constructor because it does not return the File instance (rather it returns whatever the yielded-to block returns). Grouping a method in the constructors section requires an appropriate return type restriction.

Some other class methods on File, however, are indeed constructors, e.g. the non-block overloads of .tempfile; these should be included in the list of constructors. #10584 might have taken care of them already (it doesn't seem to).

straight-shoota commented 3 years ago

Strictly speaking this might be correct. However, this method has similar characteristicts to a constructor. It creates a new instance, and instead of returning it, it just yields it to the block. It's just a constructur with built-in destructor. Both overloads are basically equivalent in their core behaviour and use case. The only difference is that using one require a call to #close afterwards (which can be in a different scope), while the other get's cleaned up with end automatically. It seems very reasonable to show both overloads closely together in the API docs, because they're so similar.

The technical difficulty is that method classification in the API docs is assigned automatically based on the return type (self indicating a constructor). The yielding overload doesn't formally qualify for that. Placing both overloads together would require a change to the doc generator. It could use some kind of annotation or directive to designate a contructor independent of the return type. Or maybe we could use self as block argument type as indication for a constructor, too. This is definitely a bigger task than just "move some copy over to a different section".

HertzDevil commented 3 years ago

Annotation is probably the best way to go since having a correct block type does not imply that the method actually constructs an instance. Plus the docs generator already has code for supporting other annotations (most notably @[Deprecated]).

straight-shoota commented 3 years ago

An annotation could also be useful for methods like String.from_utf16(pointer : Pointer(UInt16)) : Tuple(String, Pointer(UInt16)):Tuple(String,Pointer(UInt16))-class-method) which is definitely a constructor, but doesn't return self.

asterite commented 3 years ago

What if we also consider class methods that yield an instance of a type to be constructors?

HertzDevil commented 3 years ago

Then it might catch false positives like Array.each_product.

straight-shoota commented 3 years ago

There are actually both false positives and true negatives for return types, as well. I already mentioned String.from_utf16. Fiber.current returns an instance of Fiber (the type is currently not annotated), but is not a constructor. Hence I think an annotation would be a good idea. It could also allow to designate a method as being not a constructor, despite the return type. In general, the implicit semantics based on the return type make sense. And perhaps we could have something similar for block arguments. But there should be an override option.

beta-ziliani commented 3 years ago

So am I right in understanding that we need two annotations, say, @[Constructor] for constructors that do not return an element of the class, and @[NotAConstructor] for class methods returning an element of the class that is not a constructor?

straight-shoota commented 3 years ago

I'd just make it a single annotation with an optional argument: @[Constructor], @[Constructor(false)]

straight-shoota commented 10 months ago

Reflecting on this, I'm not sure anymore if @[Constructor] is the best solution here.

The heart of this issue is being able to express that a documented feature (a class method) belongs to a specific group (the constructors section of a type). This is essentially the same problem set as #1312. That issue talks about the namespace level, but the same principles apply for methods inside a type. They too would benefit from explicit grouping into sections of related items. Some sections are implicitly defined by the doc generator. And effectively we're just looking for a way to explicitly express group membership (or non-membership) for class methods belonging to the group of constructors.

I would expect a generic solution for the entire class of problems could be quite more useful than a dedicated annotation for a single instance of group membership annotation. Considering this is not a urgent issue (it's a bit annoying of course, but IMO still tolerable), I'd suggest to put some more thought into #1312 and see if we can come up with a generic mechanism that could provide a solution to this issue.

Blacksmoke16 commented 10 months ago

Unfortunately I'm not sure I'll have to time to take that discussion/refactor on. If you'd be okay with it, I could update the current PR to make that a :nodoc: annotation that we can scope the the stdlib only for now. Would fix the immediate issue, while still allowing to easily rip it out if/when a solution to #1312 is determined. Otherwise can just close it for now and see if someone else wants to pick up where I left off.

straight-shoota commented 10 months ago

This issue has been sitting for quite a while, so I'd asses it's not urgent. There's no pressure to push for an intermediary fix. Closing #14118 for now sounds good. It will be ready if we decide we'll need it in the future.