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

docs: Create a shared settings document #115

Merged
merged 4 commits into from
Sep 25, 2023

Conversation

scagood
Copy link

@scagood scagood commented Sep 11, 2023

This is the final part of #108. Previous parts: #114, #112.

Hopefully this is a better way to document shared options in this package 👀

@scagood
Copy link
Author

scagood commented Sep 11, 2023

I am not sure how to avoid this:

`no-extraneous-import` rule doc should have included rule option: include
`no-extraneous-import` rule doc should have included rule option: exclude
`no-extraneous-import` rule doc should have included rule option: replace
`no-extraneous-require` rule doc should have included rule option: include
`no-extraneous-require` rule doc should have included rule option: exclude
`no-extraneous-require` rule doc should have included rule option: replace
`no-unpublished-bin` rule doc should have included rule option: exclude
`no-unpublished-bin` rule doc should have included rule option: replace
`no-unpublished-import` rule doc should have included rule option: exclude
`no-unpublished-import` rule doc should have included rule option: replace
`no-unpublished-require` rule doc should have included rule option: exclude
`no-unpublished-require` rule doc should have included rule option: replace
`shebang` rule doc should have included rule option: include
`shebang` rule doc should have included rule option: exclude
`shebang` rule doc should have included rule option: replace

It seems to be the child options for convertPath
I have referenced the shared settings for the convertPath option in all the files 🤔

Comment on lines -54 to -56
- `exclude`: TODO
- `include`: TODO
- `replace`: TODO

Choose a reason for hiding this comment

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

@bmish it causes eslint-docs-generator to report an error, when moving the options docs to another page(shared-settings). Is there a way to ignore the error?

I'm glad to help making a PR in eslint-docs-generagor if you want. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Since it looks like the rule options are all mentioned in a shared doc instead, it should be safe for you to use the option --rule-doc-section-options false to disable the check (https://github.com/bmish/eslint-doc-generator#configuration-options).

Copy link
Author

Choose a reason for hiding this comment

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

The issue is that we mention the root option, and reference it, eg in no-extraneous-import:

We reference convertPath docs/rules/no-extraneous-import.md#convertpath and point to the shared setting docs/shared-settings.md#convertpath

The schema is defined here: lib/rules/no-extraneous-import.js#L27-L31

The issue is that the missing options are child properties of convertPath:

  • convertPath.include
  • convertPath.exclude
  • convertPath.replace
`no-extraneous-import` rule doc should have included rule option: include
`no-extraneous-import` rule doc should have included rule option: exclude
`no-extraneous-import` rule doc should have included rule option: replace

Copy link
Member

Choose a reason for hiding this comment

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

The existing eslint-doc-generator option --rule-doc-section-options is all-or-nothing in terms of checking for the options section and any named options it can detect. So with your planned design, you may just have to disable the option. Let me know if you have ideas for more granular options to control these checks.

Choose a reason for hiding this comment

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

wonderful job! The option looks useful! 👍

Choose a reason for hiding this comment

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

We can turn off this option for now and use that option when it was supported in eslint-doc-generator.
wdyt?

Copy link
Author

Choose a reason for hiding this comment

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

mmm, I don't love the option, as you'd need to be more vigilant in PRs.

If that is fine with you, then we can proceed with the option.

Copy link
Author

Choose a reason for hiding this comment

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

I don't love it, but its disabled here:

2f27eb8 (#115)

Copy link
Member

@bmish bmish Sep 22, 2023

Choose a reason for hiding this comment

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

@scagood --rule-doc-section-options-depth (as it customizes the existing --rule-doc-section-options option behavior) sounds like an interesting idea. Would you like to open an issue in eslint-doc-generator to suggest it so we can track this? I'm open to considering it if you feel that it's necessary and ideally that it would be useful to more than just this plugin.

In the meantime, you should proceed without this and just disable --rule-doc-section-options for now.

Choose a reason for hiding this comment

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

to avoid breaking changes:

  • --rule-doc-section-options=true can be treated as depth: Infinity
  • --rule-doc-section-options=false as depth: 0

@scagood scagood marked this pull request as ready for review September 21, 2023 11:22
@aladdin-add aladdin-add merged commit 7d855e6 into eslint-community:master Sep 25, 2023
15 checks passed
@scagood scagood deleted the shared-settings branch September 25, 2023 10:52
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