search

mozilla / sphinx-js

Autodoc-style extraction into Sphinx for your JS project
https://pypi.python.org/pypi/sphinx-js/
MIT License
282 stars 81 forks source link

doclets: Allow overloaded functions to not conflict #111

Closed tavianator closed 4 years ago

tavianator commented 5 years ago

Fixes #110.

erikrose commented 5 years ago
  1. This is a significant enough change that it requires new tests to demonstrate its efficacy.
  2. If there are multiple functions at the same pathname, shouldn't we, at minimum, favor the documented one, if any? It looks like the error you were experiencing happened only when trying to actually use the docs; correct me if I'm wrong. (Of course, the ultimate would be to be able to somehow specify which instance of the function we wanted, but that would need much more design work.)
erikrose commented 5 years ago

I forgot to say it, but thanks for the patch! :-) I don't use TS myself, and nobody's maintaining sphinx-js's support at the moment, so I'm very keen to have your interest.

tavianator commented 5 years ago

Sure, I'm happy to add some tests for it.

The error I was getting happens when building the docs. With the new behaviour, both overloads are listed in the docs, which is probably what you want (TypeDoc also lists both, but only one at a time, like this: https://typedoc.org/api/classes/application.html#generatedocs). Here's what the result looks like in sphinx-js: https://bistring.readthedocs.io/en/latest/JavaScript/Alignment.html#Alignment.originalBounds

The tree that reports the conflicts is used to resolve links I believe. So when you write something like :js:meth:`Alignment.originalBounds`, it has to resolve that to the actual element to link to. This change makes it just pick the first overload arbitrarily. I'm open to other behaviours here, or other ways of rendering overloads.

You're welcome for the patch! :)

graup commented 5 years ago

Tested it quickly, seems to work. This is definitely much better than throwing errors. Let's get some tests for this and merge it!

tavianator commented 5 years ago

@graup Added a simple test!

tavianator commented 5 years ago

@erikrose The code I changed doesn't distinguish between vanilla JS and TypeScript, so I'm pretty sure it won't. There's another issue as well: if you do something like .. js:autofunction:: overloaded it will only render one of them. I guess the tree needs to hold a list of matching elements rather than just one.

graup commented 5 years ago

@erikrose I'd be happy to contribute. Should we open a new issue to design an IR and coordinate working on a branch together?

There are also some problems with some other ts features currently not supported by the jsdoc representation that we could fix then.

tavianator commented 5 years ago

I think an IR would be nice. Would enable us to have TS-specific things in the documentation as first-class citizens.

To avoid needing to mess with typedoc.py at first, since we already have TypeDoc -> JSDoc, we could start with just doing TypeDoc -> JSDoc -> IR. Then later rewrite the conversion to just do TypeDoc -> IR, and compare the results on some corpus of existing TypeScript projects.

I don't have a tremendous amount of time to contribute to this but I'm happy to review designs and implementations, and maybe do some of the TS side of it.

erikrose commented 5 years ago

Alright, I started doing some of the initial spec work toward an IR over at #120. Help is welcome!

erikrose commented 4 years ago

It doesn't crash anymore as of the great IR merge of #120, demonstrated by https://github.com/mozilla/sphinx-js/blob/705659ceb08de1ec1fd16bdefd53e7b2e0a8d848/tests/test_typedoc_analysis/test_typedoc_analysis.py#L336-L344. But we should probably show multiple signatures if the Sphinx templates will let us. I've opened #153 to represent that work.