Navigation Menu

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] Re-sort all rules alphabetically #1742

Merged
merged 1 commit into from Jan 29, 2020

Conversation

ybiquitous
Copy link
Contributor

To make it easier to find rules.

It will be appreciated if you review it. 😊

…cally

To make it easier to find rules.

This adds a script to auto-generate the list of rules and embeds special tags to README.

Usage:

    $ npm run generate-list-of-rules

Check whether the auto-generated changes are committed:

    $ npm run generate-list-of-rules:check

Use `markdown-magic` package to transform README

https://www.npmjs.com/package/markdown-magic
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This looks fine, thanks - but is there any way we could add a CI check that prevents these from being mis-sorted? Otherwise they'll keep drifting out of sync.

@ybiquitous
Copy link
Contributor Author

Thanks for your review!

is there any way we could add a CI check that prevents these from being mis-sorted?

I think it is easiest to use regular expressions, so what do you think?

For example:

const pattern = /# List of supported rules([\s\S]+?)#/;
const source = '<README.md contents...>';
const ruleListText = source.match(pattern)[1];
const ruleList = ruleListText.trim().split('*');
// check sorting...

(I think using the markdown parser is too heavy for this task...)

@ljharb
Copy link
Member

ljharb commented Apr 2, 2018

I'm not sure using an X parser is ever too heavy a task to parse X :-)

@ybiquitous
Copy link
Contributor Author

To install a markdown parser such as markdown-it for the purpose of CI check seems too difficult to me. 😓

I don't know how to extract easily any list items from markdown text, what do you know it?

@ljharb
Copy link
Member

ljharb commented Apr 2, 2018

An alternative would be to make an npm run-script that generates this list automatically, and then the CI check would simply be "run the check, and fail if there's a diff".

@ybiquitous
Copy link
Contributor Author

Sorry for late reply 🙏

An alternative would be to make an npm run-script that generates this list automatically, and then the CI check would simply be "run the check, and fail if there's a diff".

Thanks, I will try it!
But diffs in this PR becomes too large, so can I do this with another PR?

@ljharb
Copy link
Member

ljharb commented Apr 10, 2018

@ybiquitous i'm not concerned with a large diff since it can be in a different commit; is it OK if we do it in this PR?

@ybiquitous
Copy link
Contributor Author

is it OK if we do it in this PR?

Yes! No problem! 🙏

@ybiquitous
Copy link
Contributor Author

This is so old. I close.

@ybiquitous ybiquitous closed this Jan 11, 2020
@ybiquitous ybiquitous deleted the readme-resort-rules branch January 11, 2020 12:09
@ljharb
Copy link
Member

ljharb commented Jan 11, 2020

That’s unfortunate; old is irrelevant. If you restore the branch and reopen the PR, and add the requested CI check, this can land.

@ybiquitous
Copy link
Contributor Author

ybiquitous commented Jan 13, 2020

@ljharb Thanks for your comment. I deleted this branch, so I retried with another branch and opened a new PR #2541.

@ljharb
Copy link
Member

ljharb commented Jan 13, 2020

There's no "restore branch" button?

@ybiquitous
Copy link
Contributor Author

I know the restore button, but I thought the branch name readme-resort-rules of this PR does not express the new PR #2541.
So, I opened as another PR.

Sorry for confusing you... 🙇

@ljharb
Copy link
Member

ljharb commented Jan 14, 2020

I'd still prefer to consolidate PR refs - the branch name doesn't stick around but the PR ref does - so it'd be great if you could reopen this, and I'll be happy to manually keep this one in sync with your new one.

@ybiquitous
Copy link
Contributor Author

OK, I will sync both. 👍

@ybiquitous ybiquitous restored the readme-resort-rules branch January 14, 2020 01:07
@ybiquitous ybiquitous reopened this Jan 14, 2020
@ybiquitous
Copy link
Contributor Author

I've cherry-picked from #2541.

@ybiquitous ybiquitous changed the title [Docs] Re-sort all rules alphabetically WIP: [Docs] Re-sort all rules alphabetically Jan 14, 2020
@ybiquitous ybiquitous changed the title WIP: [Docs] Re-sort all rules alphabetically [Docs] Re-sort all rules alphabetically Jan 14, 2020
@ybiquitous
Copy link
Contributor Author

ybiquitous commented Jan 14, 2020

@ljharb Thanks for your suggestion. markdown-magic can reduce the script code and works very well!

If we would publish and share the transform script, it might be better to publish as a markdown-magic plugin.

See also #2541 (comment)

@ybiquitous
Copy link
Contributor Author

@ljharb Could you please review this again?

@ljharb ljharb force-pushed the readme-resort-rules branch 2 times, most recently from a5a7922 to 182b045 Compare January 29, 2020 00:35
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!

@ljharb ljharb merged commit 182b045 into jsx-eslint:master Jan 29, 2020
@ybiquitous ybiquitous deleted the readme-resort-rules branch January 29, 2020 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants