Skip to content
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

Separation of constructor methods in docs #4216

Merged
merged 3 commits into from Jun 8, 2017
Merged

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Mar 31, 2017

Simple implementation of idea from issue #4071.

@jhass
Copy link
Member

jhass commented Mar 31, 2017

Mmh in general I like this, what I would even more like however is if it also would include any factory methods, so I come to wonder if this wouldn't decrease the visibility of these, since you will no longer scan the class methods in order to discover the ways you can get an instance of the class.

@Sija
Copy link
Contributor Author

Sija commented Mar 31, 2017

@jhass I'd love to include factory methods as well, alas I couldn't find any decent way to filter them out of regular class method list (except by checking return type which is not that reliable since it might be unset).

@bcardiff
Copy link
Member

bcardiff commented Mar 31, 2017

Maybe we should add something like :nodoc: to set the section.

# :factory:
# :constructor:
# :section=constructor:

@Sija
Copy link
Contributor Author

Sija commented Mar 31, 2017

👍 for :factory:

@bcardiff
Copy link
Member

NB: If a comment is applied to mark as factory, then the subtraction of methods needs to be done on class and instance methods.

class Nat
  # :factory:
  def self.zero
  end

  # :factory:
  def succ
  end
end

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Mar 31, 2017

Do we really want to start introducing some documentation-specific annotations?

Instead, what about using the specified return type of class methods (which the type inference already uses)? Maybe it could use the inferred type, too. If a class method of class X returns a X, then it's a factory.

@Sija
Copy link
Contributor Author

Sija commented Mar 31, 2017

@ysbaddaden Would be perfect, yet return type annotations are not always available IIUC.

@ysbaddaden
Copy link
Contributor

Then specifying return type of factories would have double bonus:

  • be inferred by the compiler;
  • be explicit in docs.

Eventually, could the doc generator infer types? just like the compiler does (first pass)? that seems doable, to me.

@Sija
Copy link
Contributor Author

Sija commented Mar 31, 2017

@ysbaddaden I've pushed another commit adding factory methods to the list.

@bcardiff
Copy link
Member

@ysbaddaden

Eventually, could the doc generator infer types?

Not necessarily. The type inference act upon invocation. The only invocations could be the one in the specs.

I'm ok with using an explicit return type as a flag to identify factory methods.

@Sija
Copy link
Contributor Author

Sija commented Mar 31, 2017

Anything more to do here before merging?

@asterite
Copy link
Member

@Sija I think this needs a bit more discussion, though initially I really like the idea :-)

@Sija
Copy link
Contributor Author

Sija commented Mar 31, 2017

@asterite Sure thing, I was a bit too hasty with "before merging" part ;)

@Sija Sija force-pushed the pr-4071 branch 2 times, most recently from dadae8f to 5626e2f Compare April 1, 2017 02:53
@Sija
Copy link
Contributor Author

Sija commented Apr 1, 2017

Now, when the check is more permissive, singleton methods like Fiber.current will get listed as constructors. To control this behavior we could introduce, in the vein of the first proposal, :nofactory: doc marker.

@Sija
Copy link
Contributor Author

Sija commented Apr 11, 2017

@asterite any decisions yet?

@asterite
Copy link
Member

We can separate new methods. But considering all class method that return self as constructors doesn't sound quite right. Plus, class methods are usually fewer than non-class methods, and most of them are usually constructors, so I'm not sure about this. It will bring a lot of discussions about "what is a constructor".

@Sija
Copy link
Contributor Author

Sija commented Apr 11, 2017

That's true, there are cases where this distinction can't be done automagically. To combine best of both worlds I'd go for new section titled "Factory methods" in which #new methods are added automatically + :factory: annotation which would allow adding given method to above-mentioned section.

@Sija
Copy link
Contributor Author

Sija commented May 1, 2017

@asterite ping? r we ready for some action? :)

@Sija
Copy link
Contributor Author

Sija commented Jun 8, 2017

@asterite ping :)

@asterite
Copy link
Member

asterite commented Jun 8, 2017

@Sija I like it! Thank you! 🎉

@asterite asterite merged commit 293d06f into crystal-lang:master Jun 8, 2017
@asterite asterite removed the pr:wip label Jun 8, 2017
@asterite asterite added this to the Next milestone Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants