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

Build: Automatically add rule meta data in docs (fixes #5774) #6419

Closed
wants to merge 1 commit into from

Conversation

pedrottimark
Copy link
Member

Thank you in advance for careful code review.

Added helper functions in forEach under // 4. Loop through all files in temporary directory

Main goals:

  • rules index: replace lists of active rules with tables
  • active rule doc: replace Description (rule-id) with rule-id: description in main heading, and automatically add paragraphs in recommended and fixable rules
  • other docs: add table of contents for level-2 and 3 headings if doc has at least 2 level-2 headings

An associated chore (refs #5417) will update meta.description in 10 rules which are inconsistent with the current rules index.

Future goals:

  • After writing some guidelines for editing rule docs that explain what is generated automatically, we can remove superseded content from the rules index and rule docs. To avoid problems with pull requests in progress while we make the transition, Makefile.js replaces obsolete content.
  • Make improvements to some rule descriptions.
  • In a new issue, discuss improvements to consistency of links between docs in the site: many are relative, but some are absolute.
  • After we decide about the meta data for Removed and possibly Deprecated rules, we will add that information automatically in those rule docs.

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @nzakas, @btmills and @ilyavolodin to be potential reviewers

@pedrottimark
Copy link
Member Author

Forgot to mention changes in the Jekyll front matter:

@ilyavolodin
Copy link
Member

ilyavolodin commented Jun 15, 2016

I haven't had a chance to fully go over the code yet. But I don't think this is the right direction. I think we should completely delete README.md file. Create an ejb template, provide extracted metadata as datasource and compile resulting page from the template. This would make it much easier to modify in the future.
Also, considering the amount of changes, I think we would have to separate this code into it's own file and create unittests for it, to verify that it's doing the right thing.

@pedrottimark
Copy link
Member Author

Yes, agree about unit tests, especially for rule docs because the processing must work for a moving target, until we have written the new guidelines and deleted redundant content from existing files.

@nzakas
Copy link
Member

nzakas commented Jun 15, 2016

I'd like to suggest something that keeps most of the templately stuff out of Makefile.js. In Jekyll, you can have frontmatter in files, such as:

---
ruleName: no-unused-vars
description: disallow unused variables
fixable: true
recommended: true
layout: ruledoc
---

Rest of rule docs.

Then, you can use Liquid in a layout template to use the page frontmatter like this:

# {{page.ruleName}}: {{ page.description }}

{% if page.fixable %}
fixable message
{% endif %}

{% if page.message %}
recommended message
{% endif %}

I think this is a much better way to go because it keeps page layout in the eslint.github.io repo completely. All you have to do in Makefile.js is create the frontmatter and prepend it to what's in the .md file (oh, and I guess remove the first heading).

@pedrottimark
Copy link
Member Author

@ilyavolodin I will cut this pull request back to the rule docs which was the specific request in the issue. I support an alternative method for the rules index, as long as the non-meta content for the page comes from somewhere in eslint repo. Because I lack experience with the templates, let’s invite someone else to pick up that part.

@nzakas Right, I started down that nicely declarative path when you suggested it in #5774 (comment) I got stuck because the examples of Liquid templates in the _includes and _layouts directories of eslint.github.io are in HTML. I will do more homework whether it is possible to interpret Liquid templates in markdown before conversion to HTML. That matters because descriptions can have backticks which need to be converted to code elements and removed rules (when we get to them) will have arbitrary links to replacement rule docs.

@nzakas
Copy link
Member

nzakas commented Jun 16, 2016

Oh yeah, sorry, I got a bit confused. The layout can be just HTML:

<h1>{{page.ruleName}}: {{ page.description  | markdownify }}</h1>

{% if page.fixable %}
fixable message
{% endif %}

{% if page.message %}
recommended message
{% endif %}

The markdownify filter can convert markdown to html in the template

@pedrottimark
Copy link
Member Author

Have put this on the back burner, but one question because I missed the markdownify filter:

  • Is there a way in the template to delete content other than exact match in replace filters?
  • Or does deleting main heading (and fixable paragraph, while they exist) remain in Makefile.js?

@nzakas
Copy link
Member

nzakas commented Jun 20, 2016

I don't know of a way to delete content in a Liquid template

@nzakas nzakas added the do not merge This pull request should not be merged yet label Jun 22, 2016
@pedrottimark
Copy link
Member Author

Noting while I am thinking of it that rule docs in eslint.github.io repo will be “headless”

  • Jekyll front matter
  • comment <!-- Note: No pull requests accepted for this file. See README.md in the root directory for details. -->
  • and then content written by contributors (but not including the placeholder main heading)

@pedrottimark
Copy link
Member Author

@ilyavolodin @kaicataldo Ran into errors when I added the Jekyll markdownify filter https://jekyllrb.com/docs/templates/ to an _include/rule.html file for rule docs. Have you done this before?

{{ page.description  | markdownify }}
Liquid Exception: undefined method `gsub' for nil:NilClass in _layouts/rule.html
/Library/Ruby/Gems/2.0.0/gems/kramdown-1.11.1/lib/kramdown/parser/base.rb:98:in `adapt_source': undefined method `gsub' for nil:NilClass (NoMethodError)
    from /Library/Ruby/Gems/2.0.0/gems/kramdown-1.11.1/lib/kramdown/parser/kramdown.rb:89:in `parse'
    from /Library/Ruby/Gems/2.0.0/gems/kramdown-1.11.1/lib/kramdown/parser/gfm.rb:33:in `parse'
    from /Library/Ruby/Gems/2.0.0/gems/kramdown-1.11.1/lib/kramdown/parser/base.rb:69:in `parse'
    from /Library/Ruby/Gems/2.0.0/gems/kramdown-1.11.1/lib/kramdown/document.rb:107:in `initialize'
    from /Library/Ruby/Gems/2.0.0/gems/jekyll-3.1.6/lib/jekyll/converters/markdown/kramdown_parser.rb:39:in `new'
    from /Library/Ruby/Gems/2.0.0/gems/jekyll-3.1.6/lib/jekyll/converters/markdown/kramdown_parser.rb:39:in `convert'
    from /Library/Ruby/Gems/2.0.0/gems/jekyll-3.1.6/lib/jekyll/converters/markdown.rb:65:in `convert'
    from /Library/Ruby/Gems/2.0.0/gems/jekyll-3.1.6/lib/jekyll/filters.rb:15:in `markdownify'
    from /Library/Ruby/Gems/2.0.0/gems/liquid-3.0.6/lib/liquid/strainer.rb:51:in `invoke'
    from /Library/Ruby/Gems/2.0.0/gems/liquid-3.0.6/lib/liquid/context.rb:95:in `invoke'
    from /Library/Ruby/Gems/2.0.0/gems/liquid-3.0.6/lib/liquid/variable.rb:88:in `block in render'
    from /Library/Ruby/Gems/2.0.0/gems/liquid-3.0.6/lib/liquid/variable.rb:86:in `each'
    from /Library/Ruby/Gems/2.0.0/gems/liquid-3.0.6/lib/liquid/variable.rb:86:in `inject'
    from /Library/Ruby/Gems/2.0.0/gems/liquid-3.0.6/lib/liquid/variable.rb:86:in `render'
    from /Library/Ruby/Gems/2.0.0/gems/liquid-3.0.6/lib/liquid/block.rb:151:in `render_token'```

@kaicataldo
Copy link
Member

@pedrottimark I don't have a lot of experience with Jekyll, but doing a little digging around yielded these:

Both seem to indicate that the problem was caused by something not being correct in the YAML frontmatter - any chance that's what's going on here?

@pedrottimark
Copy link
Member Author

@kaicataldo Thank you for searching. An ordinary reference to page.description succeeds:

6419 backticks

@originalfoo
Copy link
Contributor

I really like the new main heading style that puts rule name first and summary below. A superb improvement! Will the existing markdown files in docs/rules/ get updated to that format as part of this process?

@pedrottimark
Copy link
Member Author

@aubergine10 Thank you for feedback and forward-thinking question.

The long-term goal is:

  • just # rule-id for the main heading of rule docs in eslint/eslint repo and no (fixable) paragraph
  • npm run gensite will delete the main heading line and insert Jekyll front matter from rule meta data
  • store metadata for removed rules in a way that gensite can access it
  • _layouts/rule.html file in eslint/eslint.github.io repo will have Jekyll and Liquid templates

The medium-term goal will be:

  • delete description from main heading in rule docs
  • delete (fixable) paragraph in rule docs

The short-term goal of either this pull request or a better one from another team member:

  • npm run gensite will delete the main heading line and (fixable) paragraph from active rules and insert Jekyll front matter from rule meta data

@originalfoo
Copy link
Contributor

Once the (fixable) para is removed from rule docs, offline readers will be reliant on README.md. Would it be worth having additional medium-term goal that builds a fresh README.md during the build process to ensure text in that file depicts when rules are in eslint:recommended and/or (fixable)? Would mean that removed rule metadata task would have to be medium-term rather than long-term.

@vitorbal
Copy link
Member

vitorbal commented Jun 27, 2016

@pedrottimark I can reuse the script I wrote for the codeshifting the rules to the new format to generate the initial version of the file with metadata for removed rules. Just ping me when we are at that stage and I will be glad to help! :)

@pedrottimark
Copy link
Member Author

@aubergine10 Searching for text in the entire local repo is a good point to remember. I do that a lot, and it became even more effective when the original source for several non-rule docs moved from eslint.github.io to eslint repo in #6012

Although we are committed to deleting this repetition of meta data to reduce the work load on contributors and the risk of merge conflicts, the following comments suggest other ways to support offline access to the technical information:

@pedrottimark
Copy link
Member Author

@vitorbal Oh please, can you pick up the rules index with direction from with Nicholas and Ilya?

Here are a few thoughts which y’all can take or leave:

A data format to make it easy to remove a rule (if we even do that again) is copy the object value of the meta property in the rule file and paste it into a removed rules object whose keys are rule ids and values are the meta object values.

Jekyll front matter s/List of available rules/Rules/ and template which has rules class name for styles

title: Rules
layout: rules

The table format in markdown:

||(fixable)|[comma-dangle](comma-dangle)|require or disallow trailing commas
|(recommended)||[no-cond-assign](no-cond-assign)|disallow assignment operators in conditional expressions
|||[no-extra-parens](no-extra-parens)|disallow unnecessary parentheses
|(recommended)|(fixable)|[no-extra-semi](no-extra-semi)|disallow unnecessary semicolons

The table format in HTML where the description needs markdownify backticks to code elements

<table>
  <tbody>
    <tr>
      <td> </td>
      <td><span title="fixable" aria-label="fixable" class="glyphicon glyphicon-wrench"></span></td>
      <td><a href="comma-dangle">comma-dangle</a></td>
      <td>require or disallow trailing commas</td>
    </tr>
    <tr>
      <td><span title="recommended" aria-label="recommended" class="glyphicon glyphicon-ok"></span></td>
      <td> </td>
      <td><a href="no-cond-assign">no-cond-assign</a></td>
      <td>disallow assignment operators in conditional expressions</td>
    </tr>
    <tr>
      <td> </td>
      <td> </td>
      <td><a href="no-extra-parens">no-extra-parens</a></td>
      <td>disallow unnecessary parentheses</td>
    </tr>
    <tr>
      <td><span title="recommended" aria-label="recommended" class="glyphicon glyphicon-ok"></span></td>
      <td><span title="fixable" aria-label="fixable" class="glyphicon glyphicon-wrench"></span></td>
      <td><a href="no-extra-semi">no-extra-semi</a></td>
      <td>disallow unnecessary semicolons</td>
    </tr>
  </tbody>
</table>

@vitorbal
Copy link
Member

@pedrottimark sure, I will gladly take over the rules index part, i will add it to my pipeline (currently working on the internal rule to check validity of meta props) and will let you know when i start working on it!

@nzakas
Copy link
Member

nzakas commented Jun 28, 2016

@aubergine10

Would it be worth having additional medium-term goal that builds a fresh README.md during the build process to ensure text in that file depicts when rules are in eslint:recommended and/or (fixable)?

No. Forcing a build process before commit will just lead to problems. Whether or not something is fixable isn't critical information because you can only turn on fixes for all rules or turn them off for all rules. We can worry about offline docs at a later time.

@pedrottimark
Copy link
Member Author

Recording the thought that another way to reduce the complexity of code in Makefile.js is provide variables added, removed, ruleURL, docURL in Jekyll front matter for rule docs so Version and Resources sections become part of the rule.html template.

@vitorbal
Copy link
Member

@pedrottimark looks like @ilyavolodin got a head start on the rules index page, and it looks very good: 1cb0486

@ilyavolodin
Copy link
Member

Sorry I didn't say that I started working on this. I had a little bit of free time yesterday and just wanted to get it done. I'll create a PR later tonight for it.

@nzakas
Copy link
Member

nzakas commented Jul 18, 2016

Do we still need this? It looks like @ilyavolodin has a simpler solution that is almost ready.

@ilyavolodin
Copy link
Member

@nzakas I think this one does more, it also updates every rule documentation page titles and adds a paragraph about fixable, recommended.

@vitorbal
Copy link
Member

vitorbal commented Jul 18, 2016

Yes, @ilyavolodin is right, @pedrottimark has been working separately on automating the insertion of the metadata for every rule's doc!
On Mon, Jul 18, 2016 at 9:18 PM Ilya Volodin notifications@github.com
wrote:

@nzakas https://github.com/nzakas I think this one does more, it also
updates every rule documentation page titles and adds a paragraph about
fixable, recommended.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6419 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAmNdm2uOi2BAG4Z24rSyn2lEfbMGUQmks5qW9F8gaJpZM4I1sCj
.

@pedrottimark
Copy link
Member Author

What do you think if I cut this PR back even further to do only the following:

The helper function to generate lines of front matter from a config object argument seems like a good place for me to practice on setting up unit tests.

A future separate PR to meet the original goal of this PR will require more extensive unit tests for a helper function to find the redundant main heading and fixable paragraph at the beginning of rule docs.

@vitorbal
Copy link
Member

@pedrottimark Sounds good to me, that's quite some progress already :)
Just to be clear, what would be the missing tasks after this simplified PR?

@pedrottimark
Copy link
Member Author

@vitorbal Ha, the deferred part is the main goal of generating description, recommended, and fixable from rule meta data. This PR will ref 5774 instead of fix it. Reasons to cut back:

  • Will need to see if problem with markdownify for description is only my local configuration.
  • Get practice writing unit tests by separating 2 helper functions into separate PRs.

@kaicataldo
Copy link
Member

Checking in on this, since it's been sitting here for a while.

@vitorbal
Copy link
Member

Seems like @pedrottimark has been busy lately. It would still be very nice to auto-generate the metadata info of each rule doc. Would love to take this on myself, but I've got a couple of things on my pipeline already.

@nzakas
Copy link
Member

nzakas commented Oct 21, 2016

Can we close this, since it looks like it's quite old and probably won't be finished up?

@vitorbal
Copy link
Member

I'm fine with that. I can open a new issue whenever I have time to pursue this (probably only after the JSCS milestone is finished).

@alberto alberto deleted the issue5774 branch March 17, 2017 02:27
@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
archived due to age This issue has been archived; please open a new issue for any further discussion do not merge This pull request should not be merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants