hosseinmd / prettier-plugin-jsdoc

A Prettier plugin to format JSDoc comments.
MIT License
227 stars 29 forks source link

Fix empty line before list #98

Closed mtrezza closed 3 years ago

mtrezza commented 3 years ago

It seems that the change in 0.3.15 introduced an empty line before a list, which seems semantically unintuitive.

For example:

/**
 * A description.
 *
 * @param {any} var An example list:
 * -   Item 1
 * -   Item 2
 * -   Item 3
 * @returns {Promise} A return value.
 */

turns into:

/**
 * A description.
 *
 * @param {any} var An example list:
 * 
 *   -   Item 1
 *   -   Item 2
 *   -   Item 3
 * @returns {Promise} A return value.
 */

This seems semantically unintuitive, because a list rarely stands on its own, unless it is an index or table of contents.

If a list should be more clearly separated from the rest, it should add an empty line before and after the list. But that would be even worse in my opinion, because it takes up even more space. And the list cannot really be optically attached to the previous or following line anymore. For example, here the list seems pretty lost:

/**
 * A description.
 *
 * @param {any} var An example text that is
 *   long and spans multiple lines
 *   followed by a list.
 * 
 *   -   Item 1
 *   -   Item 2
 *   -   Item 3
 *
 *  And more text to come.
 * @returns {Promise} A return value.
 */

My suggestion would be to:

hosseinmd commented 3 years ago

An empty line before a list is a common behavior of md formatters like prettier md formatter. I have to say about empty line after list as well, you should add an empty line after list because if you don't add it formatter will merge that with last item of list

hosseinmd commented 3 years ago

reduce the number of empty lines to 1 if the author added more than 1 empty line before or after the list

This is currently being done

mtrezza commented 3 years ago

I don't know what the most expected / correct MD syntax is, but let's look at two popular MD / JSdoc interpreters: GitHub and Visual Studio.

A list:
- a
- b

and

A list:

- a
- b

are both displayed as:

GH:

A list:

  • a
  • b

VSC:

image

The reason seems to be that there is no other likely interpretation. A list always begins with a first item symbol (dash, bullet, number, etc), whether there is an empty line or not. That is not the case with an empty line after a list, because that has multiple likely interpretations, such as continuation of the list, continuation of the last point of the list, not part of list, etc.

From that I would conclude that forcefully adding an empty line before a list is unnecessary. Not adding an empty line before a list, would allow for compact writing. Adding an empty line seems like changing the "writing" to mimic the "displaying" which in my view is job of the interpreter, not the syntactic formatter. I therefore suggest to not force an empty line, but allow an empty line.

hosseinmd commented 3 years ago

@danielpza @RunDevelopment Please suggest.

mtrezza commented 3 years ago

To quickly mention another side effect of adding an empty line:

   /**
    * A description.
    *
    * @private
    * @param {any} This is followed by a list:
    *
    *   - List item
    *   - List item
    * @returns {any} A description.
    */

The positioning of the list is visually incorrect. The list is now closer positioned to @return than to @param even though it belongs to @param.

hosseinmd commented 3 years ago

Myabe we should keep an empty line after list for example

 /**
    * A description.
    *
    * @private
    * @param {any} This is followed by a list:
    *
    *   - List item
    *   - List item
    * 
    * @returns {any} A description.
    */
mtrezza commented 3 years ago

I think that would confirm the issue I mentioned previously, that prettify is behaving too much like an MD interpreter. Adding more and more empty lines to a comment section to correct visual issues is just blowing up the comment section without without any syntactic reason. We may as well add an empty line before the @returns to separate it more clearly from the @params, but that seems to go beyond prettification. Visual formatting is job of an interpreter I think, not of prettify.

The point is that Visual Studio for example already correctly interprets the MD without empty lines before or after the list (if the list items are indented).

In our case, JSdoc already implies that @returns is the beginning of a new section, so the list cannot continue. So why add empty lines if they are not syntactically required?

hosseinmd commented 3 years ago

We can't remove the empty line because of description like this:


/**
 * @param {any} param0 The \`Touchable\` mixin helps you handle the "press" 
 *   the geometry of elements, and observes when another responder (scroll view
 *   etc) has stolen the touch lock. It notifies your component when it should
 *   give feedback to the user. (bouncing/highlighting/unhighlighting).
 *
 *   - When a touch was activated (typically you highlight)
 *   - When a touch was deactivated (typically you unhighlight)
 *   - When a touch was "pressed" - a touch ended while still within the geometry
 *     of the element, and no other element (like scroller) has "stolen" touch
 *     lock ("responder") (Typically you bounce the element).
 *
 *   A good tap interaction isn't as simple as you might think. There should be a
 *   slight delay before showing a highlight when starting a touch. 
 *
 *   - When a touch was activated (typically you highlight)
 *   - When a touch was deactivated (typically you unhighlight)
 *
 *   subsequent touch move exceeds the boundary of the element, it should
 *   unhighlight, but if that same touch is brought back within the boundary, it
 *   should rehighlight again. A touch can move in and out of that boundary
 *   several times, each time toggling highlighting, but a "press" is only
 *   triggered if that touch ends while within the element's boundary and no
 *   scroller (or anything else) has stolen the lock on touches.
 */
hosseinmd commented 3 years ago

I agree with. You just in this situation:

/**
 * @param {any} var An example list:
 *   - Item 1
 *   - Item 2
 * @returns {Promise} A return value.
 */

If the content before of the list is one line i can remove the empty line otherwise we should add the empty line again.

/**
 * @param {any} var An example list: var An example list: var An example list:
 *   var An example list: var An example list:
 *
 *   - Item 1
 *   - Item 2
 * @returns {Promise} A return value.
 */

Is this right way?

mtrezza commented 3 years ago

The empty line would have no effect for most if not all interpreters, I believe, the question then becomes why add it. I guess the easiest would be one of these:

/**
 * @param {any} var An example list:
 * 
 *   - Item 1
 *   - Item 2
 * 
 *   In case there is more text.
 * @returns {Promise} A return value.
 */
/**
 * @param {any} var An example list without
 * more text after the list:
 * 
 *   - Item 1
 *   - Item 2
 * 
 * @returns {Promise} A return value.
 */

For b), this:

/**
 * @param {any} var An example list:
 *   - Item 1
 *   - Item 2
 * More text
 * @returns {Promise} A return value.
 */

would become this:

/**
 * @param {any} var An example list:
 *   - Item 1
 *   - Item 2 More text
 * @returns {Promise} A return value.
 */

This would stay unchanged:

/**
 * @param {any} var An example list:
 *   - Item 1
 *   - Item 2
 *
 *   More text
 * @returns {Promise} A return value.
 */

In any case, we would never get to this, which I think is visually confusing:

/**
 * @param {any} var An example list: var An example list: var An example list:
 *   var An example list: var An example list:
 *
 *   - Item 1
 *   - Item 2
 * @returns {Promise} A return value.
 */
hosseinmd commented 3 years ago

I mostly agree with (a), exactly because of consistently.

mtrezza commented 3 years ago

I think I prefer b) because it is more compact and doesn't blow up the comment section, but either way sounds good to me. Maybe a) is more MD standard and just simpler to achieve.

hosseinmd commented 3 years ago

the b) was not look good in this example:

/**
 * @param {any} param0 The \`Touchable\` mixin helps you handle the "press" 
 *   the geometry of elements, and observes when another responder (scroll view
 *   etc) has stolen the touch lock. It notifies your component when it should
 *   give feedback to the user. (bouncing/highlighting/unhighlighting).
 *   - When a touch was activated (typically you highlight)
 *   - When a touch was deactivated (typically you unhighlight)
 *   - When a touch was "pressed" - a touch ended while still within the geometry
 *     of the element, and no other element (like scroller) has "stolen" touch
 *     lock ("responder") (Typically you bounce the element).
 *
 *   A good tap interaction isn't as simple as you might think. There should be a
 *   slight delay before showing a highlight when starting a touch. 
 *   - When a touch was activated (typically you highlight)
 *   - When a touch was deactivated (typically you unhighlight)
 *
 *   subsequent touch move exceeds the boundary of the element, it should
 *   unhighlight, but if that same touch is brought back within the boundary, it
 *   should rehighlight again. A touch can move in and out of that boundary
 *   several times, each time toggling highlighting, but a "press" is only
 *   triggered if that touch ends while within the element's boundary and no
 *   scroller (or anything else) has stolen the lock on touches.
 */
mtrezza commented 3 years ago

Yes, now I see it too, let's go with a) then. I think the only issue to correct is that there is no empty line after the list if the next line starts with a tag.

/**
 * @param {any} var An example list:
 *
 *   - Item 1
 *   - Item 2
 * @returns {Promise} A return value.
 */

should become

/**
 * @param {any} var An example list:
 *
 *   - Item 1
 *   - Item 2
 *
 * @returns {Promise} A return value.
 */
hosseinmd commented 3 years ago

I will do it as soon as possible. Thank you for contributing.