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

Treat constructor method as proper constructor of an object #689

Open
lrowe opened this issue Mar 7, 2017 · 12 comments
Open

Treat constructor method as proper constructor of an object #689

lrowe opened this issue Mar 7, 2017 · 12 comments

Comments

@lrowe
Copy link

lrowe commented Mar 7, 2017

With this minimal class:

/** */
class Foo {
  /** */
  constructor(bar) {
    this.bar = bar;
  }
}

Runningnode_modules/.bin/documentation serve doc.js produces the following output with 4.0.0-beta.18:
image

I would expect the constructor not to be displayed as an instance method and it's parameters included in the instantiation example.

@leonpegg
Copy link

This is exactly what i would of expected as well also if the constructor is not defined but class is extended should pull in the def from the parent class

@paulofaria
Copy link

yeah I'm getting the same issue. is there something we should be doing that we aren't?

@tmcw
Copy link
Member

tmcw commented Mar 15, 2017

This is a known issue - so far documentation doesn't do any special treatment of constructor methods versus any other methods, and it should.

@leonpegg
Copy link

I have been working on my own theme that does additional processing of the json i will release the theme once completed, but I believe this additional processing should be done befre the theme stage

@tmcw tmcw changed the title Params on class constructor missing, constructor displayed as instance method Treat constructor method as proper constructor of an object Apr 10, 2017
@tmcw tmcw self-assigned this Apr 24, 2017
@tmcw
Copy link
Member

tmcw commented Apr 24, 2017

Q:

Should you document a class like:

/**
  * I describe the class here, and use param tags here
  */
class Foo {
  /** */
  constructor(bar) {
    this.bar = bar;
  }
}

Or

/** */
class Foo {
  /**
    * I describe the class here, and use param tags here
    */
  constructor(bar) {
    this.bar = bar;
  }
}

I'm leaning toward the former, and trying an implementation now.

@tmcw
Copy link
Member

tmcw commented Apr 24, 2017

Okay: I have a working branch that does the following:

  • In parameter inference, when it encounters an ES6 class, it looks at the class's members and, if it finds a constructor, infers parameters from that constructor.
  • Warns if a constructor is explicitly documented.

@arv
Copy link
Contributor

arv commented Apr 24, 2017

I would have preferred to document the parameters on the constructor. Putting it on the class is just more out of bounds and more likely to get out of sync.

(But personally I do not document parameters and just let the inference do it's work.)

@tmcw tmcw reopened this Apr 25, 2017
@tmcw
Copy link
Member

tmcw commented Apr 25, 2017

Do other folks feel that way?

The reason why I opted for documenting them on the class was mostly that documenting them at the class level felt like the more natural way to express all the documentation in one place, rather than running into the weird case where you have a description at the class level and param details near the constructor and they're combined, and, following that thought, cases where people use params at the class level and also at the constructor level, or descriptions on both.

Handling that indecision / merging potentially conflicting details seemed like a source of uncertainty that could be avoided, but maybe it's outweighed by the directness of params sitting near their code position?

@kbirk
Copy link

kbirk commented May 4, 2017

Just to chime in here as one random opinion, I personally prefer putting the constructor parameter documentation above the constructor and the class property documentation above the class as @arv describes. I find this easier to read and maintain, especially when the arguments don't necessarily align with the properties, for example:

/**
 * A widget object which is used to blah blah blah.
 *
 * @property {HTMLElement} element - The containing element of the widget.
 */
class Widget {

    /**
     * Instantiate a widget object.
     *
     * @param {string} selector - The string selector for the element.
     */
    constructor(selector) {
        this.element = document.querySelector(selector);
    }
}

I guess this all comes down to personal preference though.

@jamesgpearce
Copy link

In my current use case, I don't want the constructor documented - nor its parameters - because the class is never instantiated by the user (with a kind of factory pattern). Seems like even if I /** @ignore * the constructor I get all its arguments appearing, rather confusingly, as parameters at the top of the class documentation. Is there any way to avoid this if the constructor is to be ignored?

@kbirk
Copy link

kbirk commented Jul 17, 2017

Any updates on this?

@tmcw
Copy link
Member

tmcw commented Jul 17, 2017

At least for me, I've been taking a break from day-to-day work on this project in hope that other folks will join and contribute - so no updates from my side. But contributions / PRs would be incredibly welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants