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

enforce no wrapIndent for jsdoc comments #13139

Merged
merged 2 commits into from
May 15, 2024
Merged

Conversation

chriswhong
Copy link
Contributor

This PR addresses an issue that caused incorrect rendering of generated documentation on docs.mapbox.com.

The JSDoc comments for some parameters span multiple lines, and indentation of lines is enforced by the jsdoc/check-line-alignment eslint rule. This repo has additional configuration to force an indentation of 4 spaces on wrapped lines.

This indentation of subsequent lines results in markdown parsing issues. For example, the markdown to be parsed for Map.options.style looks like:

The map's Mapbox style. This must be an a JSON object conforming to
    the schema described in the [Mapbox Style Specification](https://mapbox.com/mapbox-gl-style-spec/), or a URL
    to such JSON. Can accept a null value to allow adding a style manually.
    
    To load a style from the Mapbox API, you can use a URL of the form `mapbox://styles/:owner/:style`,
    where `:owner` is your Mapbox account name and `:style` is the style ID. You can also use a
    [Mapbox-owned style](https://docs.mapbox.com/api/maps/styles/#mapbox-styles):
    * `mapbox://styles/mapbox/standard`
    * `mapbox://styles/mapbox/streets-v12`
    * `mapbox://styles/mapbox/outdoors-v12`
    * `mapbox://styles/mapbox/light-v11`
    * `mapbox://styles/mapbox/dark-v11`
    * `mapbox://styles/mapbox/satellite-v9`
    * `mapbox://styles/mapbox/satellite-streets-v12`
    * `mapbox://styles/mapbox/navigation-day-v1`
    * `mapbox://styles/mapbox/navigation-night-v1`.

The first three lines are parsed as a paragraph, but the lines after the empty line are parsed as a codeblock, and renders as such on docs.mapbox.com:

image

This PR removes the wrapIndent option, enforcing no indentation on wrapped lines. The resulting JSDoc comments were tested locally with mapbox-gl-js-docs and result in correct rendering in the docs:

image

I am not clear on the repercussions of this change to other things that make use of the JSDoc comments, if any, and seek guidance from others with more experience with this codebase on whether this change is safe.

Launch Checklist

  • Make sure the PR title is descriptive and preferably reflects the change from the user's perspective.
  • Add additional detail and context in the PR description (with screenshots/videos if there are visual changes).
  • Manually test the debug page.
  • Write tests for all new functionality and make sure the CI checks pass.
  • Document any changes to public APIs.
  • Post benchmark scores if the change could affect performance.
  • Tag @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes.
  • Tag @mapbox/gl-native if this PR includes shader changes or needs a native port.

@chriswhong chriswhong requested a review from a team as a code owner April 5, 2024 19:41
@stepankuzmin stepankuzmin self-requested a review April 6, 2024 08:23
@@ -161,7 +162,7 @@
"matchingFileName": "src/fake_filename_for_jsdoc_examples",
"rejectExampleCodeRegex": "<script>"
}],
"jsdoc/check-line-alignment": ["error", "any", {"wrapIndent": " "}],
"jsdoc/check-line-alignment": ["error"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we enforce wrapIndent with an empty string instead? So we always catch broken indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the README, the default for wrapIndent is the empty string. https://github.com/gajus/eslint-plugin-jsdoc/blob/HEAD/docs/rules/check-line-alignment.md#wrapindent

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me apart from 2 lint failures — let's get the build green before merging.

@chriswhong
Copy link
Contributor Author

@mourner checks are passing, can you merge when ready?

@stepankuzmin stepankuzmin requested a review from mourner May 15, 2024 08:18
@mourner mourner merged commit acc665f into main May 15, 2024
27 checks passed
@mourner mourner deleted the remove-jsdoc-wrap-indent branch May 15, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants