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

Add support for metadata to all rules #5417

Closed
ilyavolodin opened this issue Feb 26, 2016 · 27 comments
Closed

Add support for metadata to all rules #5417

ilyavolodin opened this issue Feb 26, 2016 · 27 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Milestone

Comments

@ilyavolodin
Copy link
Member

I've added support for new rule format and metadata to two existing rules accessor-pairs and array-bracket-spacing in v2.0.0 release.
This new format should be documented and the rest of the rules should be converted to use it. I'm not sure what would be the best way to do that, in stages or as a bulk change.

@ilyavolodin ilyavolodin added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Feb 26, 2016
@ilyavolodin ilyavolodin added this to the v3.0.0 milestone Feb 26, 2016
@platinumazure
Copy link
Member

Per the Gitter chat, I think @nzakas' vision was to do documentation first, then start converting rules accordingly. Should this issue be retitled and/or split accordingly?

@alberto
Copy link
Member

alberto commented Feb 29, 2016

Should we add a boolean property indicating whether the rule is fixable? It would help with the approach for autofixing mentioned in #5329. We'll also need it to autogenerate the docs.

@ilyavolodin
Copy link
Member Author

@alberto There's a fixable field in the metadata, but it's more granular then just Boolean. We decided it would be nice to be able to indicate groups of fixable rules for the future. We can also assume that if fixable field doesn't exist rule is not fixable.

@ilyavolodin
Copy link
Member Author

I started re-writing documentation for this today, but I'm not sure if I should replace all the old syntax with the new one, in documentation, or should I add new one, and mark old as deprecated? Since both of them supported, it feels like latter is probably more correct, but it also feels like it might confuse users.

@nzakas
Copy link
Member

nzakas commented Mar 28, 2016

I'd say create separate docs alongside the existing ones, but don't link to them. Instead, we'll use the new docs to get people started on updating the core rules. Once the core rules are updated and we've validated that the new docs are good enough for that purpose, we can link to the new rules documentation and mark the old ones as deprecated.

@ilyavolodin
Copy link
Member Author

Sounds good to me. So I'll clone "Working with Rules" documentation page and change it for the new rule structure. Should be able to take care of that this week.

@vitorbal
Copy link
Member

vitorbal commented Apr 4, 2016

Hi @ilyavolodin, I think this is a great idea! From what I understand, the final goal is to autogenerate the list of rules in the README.md file, right? I would love to help out, would there be anything you need some assistance with? :)

@ilyavolodin
Copy link
Member Author

@vitorbal The goal is to auto-generate the list of rules (at minimum), but also look into auto-generating part of rule documentation, as well as provide structure for some of the internal information. Maybe even do something about rules for plugins at some point.
After this pull request will be merged, next step would be to start converting our existing rules to new format. So far I've only converted 2 rules, mostly just to verify that my assumptions are correct, and everything is working. So 207 rules to go:-) If you want to help, we can split up the work, for example, I can start from the top and you can start from the bottom of the list. But yes, the assistance would be much appreciated.

@vitorbal
Copy link
Member

vitorbal commented Apr 5, 2016

@ilyavolodin That sounds great! Yes, I would love to contribute :) How should we coordinate it? Should we share a branch?

@ilyavolodin
Copy link
Member Author

@vitorbal I don't think we need a separate branch. Those changes are non-breaking (I already modified the core to support both old and new style). If you want to, you can start from the bottom of the rules in the lib/rules directory, and I'll start from the top (sorted alphabetically). And we'll meet somewhere in the middle.

@vitorbal
Copy link
Member

vitorbal commented Apr 8, 2016

@ilyavolodin As discussed, I created a jscodeshift transform to automate this. I ran it locally and it works like a charm. I'll send a pull request once #5719 gets merged!

@nzakas
Copy link
Member

nzakas commented Apr 8, 2016

If like to suggest that the transforms be done in batches rather than all at once to avoid merge conflicts with other PRs.

@pedrottimark
Copy link
Member

@vitorbal Concerning the excellent goal to auto-generate the rule index as you mentioned in #5417 (comment) just wanted you to know:

@ilyavolodin
Copy link
Member Author

@pedrottimark No metadata for removed rules, that would have to be a separate file somewhere. Index would be generated as part of the release script, so we will be able to add that file in as well. Format would be a simple change in the template. Rule description - yes, that we would have to change once you guys agree on the format, but again, not any more complicated then what it already is (although much bigger chance for merge conflicts:-)

@pedrottimark
Copy link
Member

In case I was unclear, am suggesting that the improved and ready-to-be-merged rule descriptions in README.md would be the source for meta.docs.description in rule files whether by editing or code transform. The rule files become the source after the index file can be auto-generated.

We can update the h1 headings in rule docs according to README.md (or rule files) in batches as part of the task to edit example sentences and option descriptions. By the way, I used schema to develop initial guidelines for option descriptions. With a consistent pattern, there is potential for automated checking that rule docs include info about all of the options.

Can you explain “bigger chance for merge conflicts” so I do not cause them by lack of understanding?

@ilyavolodin
Copy link
Member Author

@pedrottimark Ah ok, got it. So you want to merge #5778 first, and then generate metadata off of the new version, right? I know that @vitorbal had to make some manual changes (specifically around fixable attribute), but I think those were limited, so it shouldn't be a huge deal to regenerate metadata, I think.

Bigger chance for merge conflicts is due to the fact that descriptions will now live inside the rules themselves. Rules change much more often then Index.md does, hence bigger chance.

@vitorbal
Copy link
Member

vitorbal commented Apr 9, 2016

@pedrottimark @ilyavolodin hi guys, thanks for all the feedback. @ilyavolodin is right, I can easily re-generate the metadata i need for the transform, as I wrote a small script that creates a "mapping" for them by scraping the eslint.org/docs/rules page. So I would propose that we merge #5778 first and I will re-run the scraper and transform after that.

I can also do the changes in batches as @nzakas suggested. Would 40 rules per PR be okay? That would be roughly 5 PR's in total.

@pedrottimark concerning the meta-data for removed rules, I have access to those too since the scraper also picks them up. But since the transform doesn't know where to map them to, they won't be part of the PR. What's the plan for those, do we want to store them somewhere separately? I can auto-generate a file for them too if that's wanted.

@pedrottimark
Copy link
Member

@ilyavolodin Yes, #5778 is ready, I think. @vitorbal Great work, and glad you are enjoying it!

Here is content that seems to come from another source than rule meta properties:

  • h1 heading: Rules
  • introduction following h1 heading Rules in ESLint… (intro text will be rewritten soon)
  • order of the h2 headings which correspond to meta.docs.category
  • paragraph following h2 heading (will probably be rewritten soon)
  • oh, and EDIT: relationship between removed and replaced by rules

Without limiting your creativity in how to generate the rule index file, the arbitrary order makes me think of a template file like current README.md without lists/tables. Whether you could or should build tables for each section in Jekyll with Liquid templating language goes beyond my knowledge.

What about cn.eslint.org? Would its rule index file be edited as static text as en has been so far?

@vitorbal
Copy link
Member

@pedrottimark Thanks, I am definitely enjoying it :)

My experience with Jekyll and Liquid templating is also very limited, but I imagine if we decide on the table output we could have a partial that generates a table using the tablerow tag, and feed it with a JSON data file with the category -> rules mapping that could be auto-generated from the meta properties during the build process.
Or we could whip something up with EJS, like we do for the blogposts.

But of course, I would not like to step on @ilyavolodin's toes as this is his baby :) @ilyavolodin is this somewhat similar to what you had in mind?

ilyavolodin added a commit that referenced this issue Apr 12, 2016
Docs: Add a new page for working with rules (refs #5417)
vitorbal added a commit to vitorbal/eslint that referenced this issue Apr 14, 2016
Add metadata to the existing rules. Batch 2.
ilyavolodin added a commit that referenced this issue Apr 15, 2016
Update: Add metadata to existing rules - Batch 3 (refs #5417)
vitorbal added a commit to vitorbal/eslint that referenced this issue Apr 18, 2016
Add metadata to the existing rules. Batch 4.
ilyavolodin pushed a commit that referenced this issue Apr 19, 2016
Chore: Add metadata to existing rules - Batch 4 (refs #5417)
vitorbal added a commit to vitorbal/eslint that referenced this issue Apr 23, 2016
Chore: Add metadata to existing rules - Batch 5 (refs eslint#5417)
nzakas pushed a commit that referenced this issue Apr 24, 2016
Chore: Add metadata to existing rules - Batch 5 (refs #5417)
vitorbal added a commit to vitorbal/eslint that referenced this issue Apr 25, 2016
Chore: Add metadata to existing rules - Batch 6 of 7 (refs eslint#5417)
nzakas pushed a commit that referenced this issue Apr 26, 2016
Chore: Add metadata to existing rules - Batch 6 of 7 (refs #5417)
vitorbal added a commit to vitorbal/eslint that referenced this issue Apr 26, 2016
Chore: Add metadata to existing rules - Batch 7 of 7 (refs eslint#5417)
nzakas pushed a commit that referenced this issue Apr 26, 2016
Chore: Add metadata to existing rules - Batch 7 of 7 (refs #5417)
@vitorbal
Copy link
Member

@nzakas @ilyavolodin
Now that all the metadata is added, should we mark the old working-with-rules.md doc as deprecated in favor of working-with-rules-new.md?

@nzakas
Copy link
Member

nzakas commented Apr 28, 2016

Let's wait and do that on Tuesday (we have a release tomorrow and possible patch release Monday) so we can plan how to announce the change with the next release.

@vitorbal
Copy link
Member

@nzakas @ilyavolodin @pedrottimark Would love to help out and get the ball rolling for this one. How do we usually go about marking an old doc file as deprecated? Do we keep it and rename it, and link to it from the new doc that super-seeded it?

@ilyavolodin
Copy link
Member Author

@vitorbal Are you talking about working-with-rules-new? We never deprecated doc file before. Usually, we just mark something as deprecated in the documentation and put a link to the new one.

@pedrottimark
Copy link
Member

It looked to me like all the existing links to working-with-rules are better pointing to the new content, and also links on search engines. Suggest:

  • new content goes to original file name
  • original content goes to something like working-with-rules-old with maybe two links to it from working-with-rules and blog post to have links to both
  • next time there is a change which produces old content related to rules, we cut out the repetition, and organize working-with-rules-old as appendix like a backwards historical changelog of only differences.

@pedrottimark
Copy link
Member

Instead of working-with-rules-old what do you think about working-with-old-rules so the file name is consistent with the title and main heading Working With Old Rules

@nzakas
Copy link
Member

nzakas commented May 18, 2016

what I had in mind:

  1. working-with-rules renamed to working-with-rules-deprecated
  2. working-with-rules-new renamed to working-with-rules
  3. Notice at the top working-with-rules saying "if you're looking for documentation on the rule format used prior to ESLint v2.10.0, please see the deprecated documentation."
  4. A blog post explaining the new format and why we did it, and hopefully, pointing to a tool to convert existing rules

@nzakas
Copy link
Member

nzakas commented Jun 28, 2016

I'm finishing this up. Updating docs now.

@nzakas nzakas closed this as completed in 4c05967 Jun 29, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

No branches or pull requests

6 participants