Our JavaScript comments are currently in a format that neither the API module nor any well-know JS documentation tool understands. This makes it impossible to generate JS documentation the way we generate PHP documentation.

At this time, the main standard JS doc parser is JSDoc, which we can probably integrate with the API module and thus display our JS docs on the API module integration is a separate issue: #25901: Parse/save/display JavaScript files

Proposed resolution

This issue is about making sure that our JavaScript comments can be understood by JSDoc, which will incidentally help with IDEs such as PHPStorm. As a test, the patches on the child issues have been used to generate a test Drupal JS API site.

Some more information about JSDoc:

See attached screenshot for an example of how the code looks in PHPStorm.

Remaining tasks

a) Needs framework manager feedback - Agree on the strategy for the patches. Two possibilities:

1. nod_ (JS subsystem maintainer) would like to commit the minimal patches that will make sure that all our JS gets picked up by the JSDoc system and that pass some ESLint checks. The JSDoc parser does not make an entry for anything that has no doc block, so the idea is that we should first at least get doc blocks on everything with minimal information for our docs to be usable, and then go back and fix up the docs with more information.

2. jhodgdon (Documentation subsystem maintainer) is not excited about committing patches that do not conform with our docs standards, such as having "@param {type} var_name" not followed by descriptive docs. She is afraid that people will start using these non-conforming doc blocks as templates for future docs, and this problem will be perpetuated. She'd rather see the conforming docs added incrementally, rather than adding non-conforming doc blocks, although she recognizes that because JSDoc only picks up things with doc blocks, this would be a problem.

We need to decide on which approach to take.

b) Once (a) is decided, get the patches on the child issues reviewed and committed.

User interface changes

None. DX is improved if developers eventually have better access to JS docs on, and IDEs can use them.

API changes


#47 jsdoc.png79.2 KBmarkcarver
#36 interdiff.txt311.33 KBnod_
#36 core-jsdoc-2182153-36.patch427.97 KBnod_
#27 misc-eslint-jsdoc.txt20.47 KBnod_
#27 eslint-jsdoc-do-not-test.patch362 bytesnod_
#27 core-jsdoc-2182153-27.patch128.32 KBnod_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,600 pass(es). View
#27 interdiff.txt33.01 KBnod_
#26 core-jsdoc-2182153-26-misc.patch117.05 KBnod_
#26 interdiff.txt6.51 KBnod_
#25 interdiff.txt33.02 KBnod_
#25 core-jsdoc-2182153-25-misc.patch114.05 KBnod_
#22 core-jsdoc-2182153-22-misc.patch109.17 KBnod_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,468 pass(es). View
#21 core-jsdoc.patch104.32 KBnod_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,430 pass(es). View
#13 core-jsdoc-2182153-13.patch295.46 KBnod_
jsdoc.png609.01 KBnod_
core-jsdoc.patch257.31 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
Members fund testing for the Drupal project. Drupal Association Learn more


Status: Needs review » Needs work

The last submitted patch, core-jsdoc.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

Cancelled patch test, it's only changing JS files.

seutje’s picture

Holy moley this thing is huge, nearly crashes my Chromesie when loading it up in Dreditor...

I only managed to scroll through about a third of it so far, so consider this partial, at best.

  1. @@ -1,6 +1,7 @@
    - * Limits the invocations of a function in a given time frame.
    - *
    + * @file
    + */

    Some of these seem to replace the comment with an empty @file block, or am I missing something?

  2. +++ b/core/misc/ajax.js
    @@ -1,28 +1,29 @@
    -    function loadAjaxBehavior(base) {
    +    function loadAjaxBehavior (base) {
           var element_settings = settings.ajax[base];

    Does this whitespace change pertain to the JSDoc-ification in particular?

  3. +++ b/core/misc/dropbutton/dropbutton.js
    @@ -36,21 +41,29 @@ function dropbuttonClickHandler (e) {
    + *   visual UAs.
    -function DropButton (dropbutton, settings) {
    +Drupal.DropButton = function (dropbutton, settings) {
       // Merge defaults with settings.

    Sneaking in some secret optimalisations? All fine by me, but our test coverage isn't all that, so something this big will need quite a bit of testing.

I'll try to run through the rest later today or somewhere this week.

oresh’s picture

@seutje, about the spaces before brackets. I think nod_ is trying to follow js coding standards but sometimes it's just personal preferences :).
As told here:

There should be no space between the function name and the following left parenthesis.
Exception: If a function literal is anonymous, there should be a single space between the word "function" and the left parenthesis "(". Otherwise, it can appear that the function's name is actually "function".

About your .1 - I also saw that missing line, but it's declaed a bit lower in @summary or something like that :)

My results of first 1k lines.

1. I'm not sure if this is a jQuery object. In function parameters in comment it says: @param {HTMLElement} element. A bit lower there's this.$form which is a jQuery object.

+++ b/core/misc/ajax.js
@@ 129
+ * @param {Object} element_settings
 Drupal.ajax = function (base, element, element_settings) {
@@ 159
+  /**
+   * @type {jQueryObject}
+   */
   this.element = element;
@@ 172
+     * @type {jQueryObject}
+     */
     this.$form = $(this.element.form);

2. Not sure why you changed it 'ajax' to 'this', when we defined ajax as this before.

+++ b/core/misc/ajax.js
@@ 213
  var ajax = this;
-  ajax.options = {
@@ 230
+  this.options = {

3. Drupal.autocomplete is not commented in misc/autocompete.js

4. Drupal.behavior.collapse misses description. Something like:
"Adds html5 details element functionallity/behavior for old browsers"

5. Function comment is broken one.

+++ b/core/misc/dialog.js
@@ 603
+  /**
+   * When using this API directly (when generating dialogs on the client side),
+   * you may want to override this method and do
+   * as well, to remove the dialog on closing.
+   * @example

6. No need in @private, as discussed :)

+++ b/core/misc/displace.js
@@ 770
+   * @private
+   * @function
+   * @return {OffsetObject}

Next 1k lines review coming soon.

nod_’s picture

Yeah sorry I left some code changes in the patch. I'll leave only the ones that are related to documenting things tonight.

nod_’s picture

Also JS code standards are not up to date, I added a link to #1778828: [policy, no patch] Update JS coding standards on the doc page to warn people.

nod_’s picture

Issue summary: View changes
jhodgdon’s picture

Would it be possible to make this a little more similar to our PHP docs standards?

For instance:

  * Extends Error to provide handling for Errors in AJAX
+ * @constructor
+ * @augments Error
+ * @param {xhr} xmlhttp
+ * @param {String} uri
 Drupal.AjaxError = function(xmlhttp, uri) {

Normally in our PHP docs we have blank lines between each different type of @ tag. Also, we require that parameters actually describe what they are -- I believe JSDoc support this? These @param descriptions are not actually documentation -- they are just repeating what you can see in the function declaration.

nod_’s picture

Sure I can work the whitespacing to be closer to our PHP standards, not an issue.

It's possible to document what the parameters are (there are a few examples in other files), though they are not without use even like this. The type can be picked up by IDE to validate and give hints on things that can be done with the variable inside the function.

I can't say I'll have every single parameter commented though, there are too many of them to do it in a timely fashion. Patch needs constant reroll and keeping it around for a long time isn't something I look forward to.

jhodgdon’s picture

Perhaps before this patch goes in, we should finalize the standards then? And then rather than one big patch that attempts to comment all the JS it should be split into several smaller issues that are not so hard to reroll?

webchick’s picture

Priority: Normal » Major

We do not historically have a good track record at all of landing multiple smaller issues in anything approaching a timely manner. Now that we do "destabilization days" following an alpha, I'd personally rather get this in as one big patch and do further clean-ups later. Once the code exists in HEAD it's much easier to clean up.

Agreed though that we should get the details nailed down before commit so that the change notice is accurate and can be pointed to in Novice issues to perform further clean-ups.

Also, this feels at least major, if not critical, to me. We currently have zero documentation of JS on api.d.o from what I understand, and while getting that in place is a separate problem/issue, given how much more JS there is in D8 than any release previous, this patch is a rather big deal IMO.

jhodgdon’s picture

Status: Needs review » Postponed

Well I don't think we can know how to review this patch to see if it complies with our JS standards until we have JS standards for docs.

We have two issues:
#1337022: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser
#1778828: [policy, no patch] Update JS coding standards

On one of those issues we need to decide what the standards are and adopt them, and then we can return here and make/review/commit a patch/

nod_’s picture

295.46 KB

Just a reroll because of #1913510: Core Javascript Files Not Using Standard Indentation to avoid really painful merges later on.

Wim Leers’s picture

#2174589: Split up ckeditor.admin.js has landed and AFAIK was the last blocker to this.

Wim Leers’s picture

Status: Postponed » Active
YesCT’s picture

Issue summary: View changes
Status: Active » Needs work
Issue tags: +Needs reroll

I think since there was a patch (is a patch, but it does not apply) this is needs work... reroll.

nod_’s picture

Assigned: Unassigned » nod_
jhodgdon’s picture

Title: Use JSDoc » Document Drupal JavaScript using JSDoc

Fixing title.

A few notes on the previous patch:

+ * @file
+ * Provides Ajax page updating via jQuery `$.ajax`.
+ *
+ * Ajax is a method of making a request via JavaScript while viewing an HTML
+ * page. The request returns an array of commands encoded in JSON, which is
+ * then executed to make any changes that are necessary to the page.
+ *
+ * Drupal uses this file to enhance form elements with #ajax['path']
+ * and #ajax['wrapper'] properties. If set, this file will automatically be
+ * included to provide Ajax capabilities.
+ */
 (function ($, window, Drupal, drupalSettings) {

A @file doc block should always have a blank space after it, or else most docs parsers would think the doc block is documenting the next line of code.

    * Attaches the Ajax behavior to each Ajax form element.
+   * @type {Behavior}
   Drupal.behaviors.AJAX = {

I think there should be a blank line before @type too, and generally after the summary line, having a blank line would be good?

    * Extends Error to provide handling for Errors in AJAX
+   * @constructor
+   * @augments Error
+   * @param {xhr} xmlhttp
+   * @param {String} uri
   Drupal.AjaxError = function (xmlhttp, uri) {

It would be good to establish an order for the @tags in the doc blocks, even if JSDoc doesn't require it, for consistency. And to leave blank lines between different types of @tags, for human readability. In our PHP docs standards, we have similar standards.

There should also be a blank line before each doc block, like:

     this.message = statusCode + pathText + statusText + responseText + readyStateText;
+    /**
+     * Set to `AjaxError`, used by some browser to display a more accurate
+     * stacktrace.
+     * @type {String}
+     */ = 'AjaxError';

Also let's confine this patch to JSDoc and not make other changes like:

-    } catch (e) {}
+    }
+    catch (e) {}
nod_’s picture

Thanks for the feedback. I picked up the patch last week and started rerolling it (and fix the 80 char limit in the process). I'll add all that in.

Side note, there were improvements on jsdoc side, hopefully we won't have to support a custom jsdoc theme or at least it be less involved than what I have now for generating the doc pages.

jhodgdon’s picture

Yeah, that would be good! The issue for adding JS parsing to the API site is:
#25901: Parse/save/display JavaScript files
(already listed as "related"). If you have any updates on how to get that to happen, that would be the place to add them. Thanks!

nod_’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
104.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,430 pass(es). View

Hold back my great need of refactoring, only touched the comments for the misc folder. I'll continue later on.

  • Full JSdoc for almost everything exposed to users (missing documentation of events firing and listening).
  • 80 char comment rule followed. We destroyed that a long time ago when the files were properly indented (adding 2 spaces on everything).

JSDoc got a new version out (v3) which makes things better, with the default template and this patch we can have something decent, phpstorm likes it too. Some docs are inconsistent because our code is inconsistent, can't do much about that in this issue. Good first step though.

nod_’s picture

109.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,468 pass(es). View

forgot to put a new line before /** in a couple of place.

Forgot to say that the patch is fixing the feedback in #18 too

1. Added an empty line after @file docblock
2. Added a new line between doc tag types
3. About the order or tag, haven't thought much about it, always end up doing the same. Will need to look and get a feel of what looks better.
4. All the extra code reformatting and messing with the js code is gone from the patch.

jhodgdon’s picture

Status: Needs review » Needs work

Looking better!

So... It's pretty clear that we need JSDoc standards, and I would hope they would be in line with the PHP standards we already have. The issues establishing the standards are not finalized, so I'm not sure why we're proceeding with this patch already... so I've evaluated this based on an assumption that any JS standards we adopt would be in line with the PHP standards we have (plus or minus differences between the languages).

Here are some notes -- I didn't get through the whole patch, but most of them apply in multiple places.

  1. +++ b/core/misc/ajax.js
    @@ -1,19 +1,22 @@
    + * @file
    + *
    + * Provides Ajax page updating via jQuery $.ajax (Asynchronous JavaScript and
    + * XML).
    + *

    Would it be possible to follow the general PHP doc block and @file block standards we have had in place for years?

    So here, it would be:

     * @file
     * One line description.
     * Rest of stuff.
  2. +++ b/core/misc/ajax.js
    @@ -81,6 +85,14 @@
    +   * @param {XMLHttpRequest} xmlhttp
    +   *   xhr object.
    +   * @param {string} uri
       Drupal.AjaxError = function (xmlhttp, uri) {

    What is an "xhr object"? I truly have no idea what it is.

    Normally in docs, acronyms would be all caps too, so it would be "XHR" presumably?

    The second param also is lacking docs.

    In this case, I think it would be sensible to have something like "The URI where the error occurred" or whatever the URI is, for the doc line.

  3. +++ b/core/misc/ajax.js
    @@ -125,7 +138,18 @@
    +     * Formated and translated error message.

    Formatted is misspelled here.

  4. +++ b/core/misc/ajax.js
    @@ -125,7 +138,18 @@
    +     * Used by some browser to display a more accurate stacktrace.

    browser -> browsers

    Also I think stack trace is two words?

  5. +++ b/core/misc/ajax.js
    @@ -142,6 +166,16 @@
    +   *   id of element the event is binded to.

    Should be ID not id. And binded -> bound.

  6. +++ b/core/misc/ajax.js
    @@ -142,6 +166,16 @@
    +   *   DOM element uesd to find the ajax url.

    - uesd -> used
    - Shouldn't Ajax be capitalized?
    - url -> URL

  7. +++ b/core/misc/ajax.js
    @@ -142,6 +166,16 @@
    +   *   Settings for the Drupal Ajax Object.

    Object should not be capitalized here.

  8. +++ b/core/misc/ajax.js
    @@ -180,15 +217,30 @@
    +      /**
    +       * @type {string}
    +       */
           this.wrapper = '#' + this.wrapper;

    No description/docs?

  9. +++ b/core/misc/ajax.js
    @@ -180,15 +217,30 @@
    +    /**
    +     * @type {HTMLElement}
    +     */
         this.element = element;

    No docs here. Do we have standards now? In PHP docs land we require a description on every doc block at a minimum. Should have the same requirement for JS.

  10. +++ b/core/misc/ajax.js
    @@ -200,33 +252,56 @@
    +    /**
    +     * Replacing 'nojs' with 'ajax' in the URL allows for an easy method to let
    +     * the server detect when it needs to degrade gracefully.
    +     * There are four scenarios to check for:
    +     * 1. /nojs/
    +     * 2. /nojs$ - The end of a URL string.
    +     * 3. /nojs? - Followed by a query (e.g. path/nojs?destination=foobar).
    +     * 4. /nojs# - Followed by a fragment (e.g.: path/nojs#myfragment).
    +     */
         this.url = this.url.replace(/\/nojs(\/|$|\?|#)/g, '/ajax$1');

    This is not documenting a JS object or variable, so it should not be a /** comment, just a // comment.

  11. +++ b/core/misc/ajax.js
    @@ -200,33 +252,56 @@
    +     * @prop {String} url
    +     * @prop {Object} data
    +     * @prop {Function} beforeSerialize
    +     * @prop {Function} beforeSubmit
    +     * @prop {Function} beforeSend
    +     * @prop {Function} success
    +     * @prop {Function} complere
    +     * @prop {String} dataType
    +     * @prop {Object} accepts
    +     * @prop {String} accepts.json
    +     * @prop {String} type
    +     */
         ajax.options = {
           url: ajax.url,

    All of these should have docs?

  12. +++ b/core/misc/ajax.js
    @@ -507,7 +583,8 @@
    -   * Build an effect object which tells us how to apply the effect when adding new HTML.
    +   * Build an effect object which tells us how to apply the effect when adding
    +   * new HTML.
       Drupal.ajax.prototype.getEffect = function (response) {
         var type = response.effect || this.effect;

    Our PHP docs standards require the first line after a /** to be one complete sentence ending in . and less than 80 chars, and then any other docs to be a new paragraph. Presumably we should do the same in JS?

  13. +++ b/core/misc/ajax.js
    @@ -557,10 +634,14 @@
    -   * Provide a series of commands that the server can request the client perform.
    +   * Provide a series of commands that the server can request the client
    +   * perform.
    +   *
    +   * @constructor
       Drupal.AjaxCommands = function () {};
       Drupal.AjaxCommands.prototype = {

    Same here?

  14. +++ b/core/misc/announce.js
    @@ -1,9 +1,12 @@
    + * @example

    Should this just be @code? I saw that earlier in the patch.

nod_’s picture

I'm moving things along because now most of the code has finished moving around (all the issues about splitting the toolbar, editor module js code in separate JS files are done) it's not crazy to chase HEAD with this patch. As webchick said in #11, it's a first step so while I agree we should document everything, it's just not possible to do in this patch. Right now the documentation is inconsistent and break many rules, sometimes there are {} around the variable type, sometimes not, sometimes there is no type or no documentation at all. After the patch things will be consistent, not complete.

About the standards, I consider the js CS done, ESlint checks for everything important. As for the doc standards there is a page it's not completely right (it's 3 years old after all) but it isn't wrong either (and looking back on it, I haven't followed some stuff in my patch), it's from a comment you made in #1337022-63: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser.

@code doesn't exist in JSDoc, it's @example.

I'll update the patch based on feedback from #23

nod_’s picture

114.05 KB
33.02 KB

Some more fixes. I think I got all the places where the first sentence was wrapping and I made it fit.

nod_’s picture

6.51 KB
117.05 KB

Added most of our custom events documentation.

nod_’s picture

Status: Needs work » Needs review
33.01 KB
128.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,600 pass(es). View
362 bytes
20.47 KB

Reroll for #1533366: Simplify and optimize Drupal.ajax() instantiation and implementation

Still working only on the core/misc folder only.

Moved some types around to make the output more handy to read. Added type to all behaviors.

As far as tag order is concerned this order looks pretty good (looked at the PHP one to keep somewhat the same order).








For the typedef group (typedef, var, name, namespace, constructor, callback, event, function) the order inside that group doesn't matter since there should only be one of those tag per docblock maximum.

As for fears of having incomplete docs: ESLint can check for validity of JSDoc comments, once this patch gets in we will turn that rule on, with the settings you can see in the eslint-jsdoc-do-not-test.patch. I attached the corresponding output from eslint as an example of running this configuration on core/misc after this patch is applied. We can require everything to have a description in a follow-up.

jhodgdon’s picture

I was going to review the child issues, but I am not familiar enough with the JSDoc standards that apply. Like, what does this line mean?

+   * @type {Drupal~behavior}
   Drupal.behaviors.activeLinks = {

I have no idea what this means, or what the proper syntax is, so it is difficult for me to review it... some other @type lines just say {string}, but this one has a ~ in it, hum... not sure what it means.

Then I went to this issue's summary and also didn't find any links to the proposed standards that the docs are supposed to comply with or what the tags mean. I'm also not clear for the child issues on exactly which docs standards the patches are supposed to be fixing and which ones are being left for later issues.

So can we get a quick issue summary update on this issue with links to the standards, and on the child issues to tell me what I'm supposed to be looking for and what I should let slide in my review? Thanks!

jhodgdon’s picture

jhodgdon’s picture

Oh I guess the child issues do tell me what I'm supposed to review for. I just need info on the JSDoc ins and outs and our standards. Thanks!

Wim Leers’s picture

So much good stuff here. Amazing work, nod_!

Also very cool that ESLint is able to check the validity of JSDoc comments :) Soon, we'll be wishing we had PHPLint :)

@jhodgdon Regarding that tilde sign: I suspect it's a typo, because it's not mentioned here either:

nod_’s picture

Issue summary: View changes

Updated IS.

I believe you're looking for this page of the JSDoc Using namepaths about the ~. (it's not a typo) Basically it's to say the behavior type belongs to the Drupal namespace (then it'll show on the Drupal namespace doc page).

I updated the doc page

I also generated the documentation for the latest version of the patches: Drupal JS API. Contrary to last year, this time I only had to use an existing jsdoc template, change 2 css values and it's already usable. There are a bunch of others but that was the only complete enough with search.

Wim Leers’s picture

I literally stared at my screen with open mouth at the awesomeness of

attiks’s picture

Amazing work, are we still aiming on integrating it into the php api so we can cross reference?

nod_’s picture

nod_’s picture

Title: Document Drupal JavaScript using JSDoc » [Meta] Document Drupal JavaScript using JSDoc
Status: Needs review » Active
427.97 KB
311.33 KB

Just uploading the latest fixes, will split that up in the sub issues later.

I added the eslint rule since I fixed all eslint warnings \o/. I did turn off the required description for parameters and returns options because sanity. We will turn that on once this first step gets cleared and we can throw people at it.

nod_’s picture

Added documentation on a couple of important internal functions. All child issues are up to date, my API doc site has been updated.

Created #2494177: Enable ESLint warning for missing JSDoc to isolate the eslint rule activation patch. I looked at the eslint queue, they're considering adding some more rules around jsdoc, mainly making sure things start with a capital letter and end with a ., we'll activate it when it gets in eslint. I guess we can probably contribute to add a rule about tag order and new line formating around those tags.

nod_’s picture

Issue summary: View changes
Wim Leers’s picture

Status: Active » Needs review
nod_’s picture

Status: Needs review » Active

See child issues for review.

jhodgdon’s picture

A general comment:

I would much much much rather see a smaller patch that really fixes up the docs for a smaller number of things, rather than a big patch that is really difficult to review (on account of size) and doesn't fully fix up the docs. It's hard to commit docs that are not following our own standards, like adding @param without a description -- I'd rather just leave it out until someone has time to add a description instead of adding in a non-compliant doc block.

nod_’s picture

reply to the more general questions from #2493677-8: JSDoc for misc/ files :

My plan was:

  1. get all the doc consistent and useful
  2. then enable ESLint description check and fix all the param, prop, return documentation

It's insane to document all of them now, and if we do it little by little we just can't generate any useful documentation: it's useless to PHPStorm which means nobody will see the documentation and it will take years before the codebase is fixed.

I mean at some point we have to draw a line between being right or being useful. I'm arguing we need to be useful first and that's what my patches reflect. As a Js developer I want the Drupal JS API not to be a black hole. What's best live with, a codebase where, within the same module you have:

   * A toggle is an interactive element often bound to a click handler.
   * @return {String}
   *   A string representing a DOM fragment.
  Drupal.theme.toolbarMenuItemToggle = function (options) {

// and 

     * Announces an orientation change.
     * @param Drupal.Toolbar.ToolbarModel model
     * @param String orientation
     *   The new value of the orientation attribute in the model.
    onOrientationChange: function (model, orientation) {

Docbock using brackets for type, and no brackets and no description for @param (that's clearly not following anything) or a codebase where everything is marked up but not fully described (yet)? Then we can have novice fill in the blanks, great novice sprint tasks.

Do you want to keep that around for years or would you rather something consistent? I mean It's been like this for ever and nobody ever fixed it. I'm offering a patch that will make the doc visible, useful and that will make people actually read it. Which is how this doc is ever going to be accurate because right now and for the last 9 years, it's been useless.

nod_’s picture

Updated my jsdoc api site.

nod_’s picture

If that can help with fears of un-described params and returns I do have a good track record of getting ESLint errors fixed and sticking with unthankful patches #1415788: Javascript winter clean-up.

attiks’s picture

#42 For what it is worth I agree with #43, it might not be following the guidelines, but at least it will provide us with something that we can use.

markcarver’s picture

79.2 KB

I definitely agree with #43.

It's hard to commit docs that are not following our own standards, like adding @param without a description

That's because these are standards, not "ours" per se. Also, for IDEs like PHPStorm, having a @param with no description is far better than no @param at all. It doesn't show you the description in autocomplete anyway.

There's a screenshot in the issue summary of why this issue is important, but I'll create another one to see if it helps clear it up a little better:

jhodgdon’s picture

Issue summary: View changes
Issue tags: +Needs framework manager review

I understand that an @param with a type and no description is better than not having an @param. My worry is just that these look like complete doc blocks to people and they'll be copy-pasting them as templates, and that the incomplete docs will be propagated. So I'm generally against committing wholesale "improve the docs" patches that add non-compliant docs.

I think we should get the opinion of a decision-maker on this, and I don't think I am the final decision-maker on this, because it affects both the JavaScript and Documentation subsystems. So we need the Framework Manager to weigh in, following the logic in #2457875: [policy] Evolving and documenting Drupal core's structure, responsibilities, and decision-making. [The current framework managers are effulgentsia, catch, and alexpott.]

Adding tag, and adding note to issue summary about what decision needs to be made. Actually, updating the whole issue summary and adding a template.

markcarver’s picture

I was going to say a whole lot more on this, but it's just not worth it anymore. Suffice it to say, some of it is in this comment #1337022-100: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser.

Btw, I also find it rather tacky and a moot point writing "@nod_'s argument" for him (which is mostly negated and generally steers in the direction of #2 anyway).

jhodgdon’s picture

@markcarver: I figured that if @nod_ disagreed with what I wrote, he'd update the issue summary, although I guess I didn't say that explicitly. Sorry if I stepped on toes, I was just trying to keep things moving and avoid stalling everything for forever. You seem to be kind of angry or disappointed or something... I hope we can keep the discussion constructive here, figure out what's best to do, and get it done.

droplet’s picture

I'm not worrying about `no description`. Instead, I'm more worrying about if we make it *mandatory* policy, we will adding meaningless comments, eg "A string is String".

While documenting files is not popular in JS world, I could imagine any extra *crazy* Drupal Rule will kick contributors out of Drupal Community.

jhodgdon’s picture

nod_’s picture

Issue summary: View changes
alexpott’s picture

So I'm not sure that this really is a framework manager issue. Afaics we have a disagreement about the approach to take between @nod_ (JS maintainer) and @jhodgdon (Docs maintainer).

Personally (non-framework manager hat on), I think given the scale of the changes that we need to do to fully implement a JSDoc standard, it would be good to go with @nod_'s proposal because then we get a working API site and people will be encouraged to file patches to flesh out the information. I understand @jhodgdon's concerns - we do do a lot of c&p errors with PHP but doing this incrementally will allow us to make quicker progress and settle the standards whilst using them - so we'll have less theoretical discussions.

jhodgdon’s picture

OK, that's good enough for me. Let's proceed then. Thanks!

nod_’s picture

nod_’s picture

Status: Active » Fixed

Rejoice! Initial patches are in! let's start the clean-up in #2501679: Document JavaScript.

I'll be updating periodically my own jsdoc site (it's manually updated frequently) until #2501135: Add JSDoc site to is up and running. Once it's on d.o I'll kill my site.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.