Skip to content

docs(eslint-plugin): correct issues in documentation #1396

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

Merged
merged 7 commits into from
Jan 2, 2020
Merged

docs(eslint-plugin): correct issues in documentation #1396

merged 7 commits into from
Jan 2, 2020

Conversation

armano2
Copy link
Collaborator

@armano2 armano2 commented Jan 1, 2020

  • description in documentation should match rule metadata
  • validate if header in docs matches "# {description} (`{name}`)"
  • fix link to docs for some rules

i was also thinking about replacing this custom validation with jest test case

Test file

This code does same checks as `validate-docs`

import { promisify } from 'util';
import fs from 'fs';
import path from 'path';

import marked from 'marked';
import rules from '../src/rules';

// Promise api of fs is available for node 10+, for node 8 we have to use util
const existsAsync = promisify(fs.exists);
const readFileAsync = promisify(fs.readFile);
const readdirAsync = promisify(fs.readdir);

const docsRoot = path.resolve(__dirname, '../docs/rules');
const rulesData = Object.entries(rules);

function createRuleLink(ruleName: string): string {
  return `[\`@typescript-eslint/${ruleName}\`](./docs/rules/${ruleName}.md)`;
}

function parseReadme(): marked.Tokens.Table {
  const readmeRaw = fs.readFileSync(
    path.resolve(__dirname, '../README.md'),
    'utf8',
  );
  const readme = marked.lexer(readmeRaw, {
    gfm: true,
    silent: false,
  });

  // find the table
  const rulesTable = readme.find(
    token => token.type === 'table',
  ) as marked.Tokens.Table;
  if (!rulesTable) {
    throw Error('Could not find the rules table in README.md');
  }

  return rulesTable;
}

describe('Validating rule docs', () => {
  it(`check if all files are present`, async () => {
    const files = await readdirAsync(docsRoot);
    const ruleFiles = Object.keys(rules)
      .map(rule => `${rule}.md`)
      .sort();

    expect(files.sort()).toEqual(ruleFiles);
  });

  it.each(rulesData)(
    'Description of %s.md must match',
    async (ruleName, rule) => {
      const filePath = path.join(docsRoot, `${ruleName}.md`);
      expect(await existsAsync(filePath)).toBeTruthy();
      // validate if description of rule is same as in docs
      const file = await readFileAsync(filePath, 'utf-8');
      expect((/^\s*#[^\r\n]*/.exec(file) ?? [])[0]).toEqual(
        `# ${rule.meta.docs.description} (\`${ruleName}\`)`,
      );
    },
  );
});

describe('Validating rule metadata', () => {
  describe.each(rulesData)('%s', (ruleName, rule) => {
    it(`name field in rule must be correct`, () => {
      // validate if rule name is same as url
      // there is no way to access this field but its used only in generation of docs url
      expect(rule.meta.docs.url.endsWith(`rules/${ruleName}.md`)).toBeTruthy();
    });

    it('Expects type checking to be set for rule', async () => {
      // quick-and-dirty check to see if it uses parserServices
      // not perfect but should be good enough
      const ruleFileContents = await readFileAsync(
        path.resolve(__dirname, `../src/rules/${ruleName}.ts`),
      );

      const usesTypeInformation =
        ruleFileContents.includes('getParserServices') || undefined;
      expect(usesTypeInformation).toEqual(rule.meta.docs.requiresTypeChecking);
    });
  });
});

describe('Validating README.md', () => {
  const rulesTable = parseReadme().cells;
  const notDeprecated = rulesData.filter(
    ([, rule]) => rule.meta.deprecated !== true,
  );

  it('Checking table structure...', () => {
    const ruleNames = notDeprecated
      .map(([ruleName]) => ruleName)
      .sort()
      .map(createRuleLink);

    expect(rulesTable.map(row => row[0])).toStrictEqual(ruleNames);
  });

  describe.each(notDeprecated)(`Checking rule %s`, (ruleName, rule) => {
    const ruleRow =
      rulesTable.find(row => row[0].includes(`/${ruleName}.md`)) ?? [];

    it(`Link column should be set correctly`, () => {
      expect(ruleRow[0]).toEqual(createRuleLink(ruleName));
    });

    it(`Description column should be set correctly`, () => {
      expect(ruleRow[1]).toEqual(rule.meta.docs.description);
    });

    it(`Recommended column should be set correctly`, () => {
      expect(ruleRow[2]).toEqual(
        rule.meta.docs.recommended ? ':heavy_check_mark:' : '',
      );
    });

    it(`Fixable column should be set correctly`, () => {
      expect(ruleRow[3]).toEqual(
        rule.meta.fixable !== undefined ? ':wrench:' : '',
      );
    });

    it(`Requiring type information column should be set correctly`, () => {
      expect(ruleRow[4]).toEqual(
        rule.meta.docs.requiresTypeChecking === true ? ':thought_balloon:' : '',
      );
    });
  });
});


Q: How header in documentation for rule should look like? (right now we are its random)

  • # {description} (`{name}`)
  • # {description} ({name})

- description in documentation should match rule metadata
- validate if header in docs matches "# {description} (`{ruleName}`)"
- fix link to docs for some rules
@typescript-eslint

This comment has been minimized.

@armano2 armano2 added the documentation Documentation ("docs") that needs adding/updating label Jan 1, 2020
@bradzacher
Copy link
Member

Thanks for doing this!
a while back I started writing a validator for the rule docs, but it was too much work at the time.
This is a great interim step!

@armano2
Copy link
Collaborator Author

armano2 commented Jan 2, 2020

ok, i applied requested changes,

bdw. what do you think about my idea about moving this out to jest test?


i just saw 3256f19 👍

@bradzacher
Copy link
Member

Sorry, I missed the description.

How header in documentation for rule should look like?

You've done it right - # description (`rule-name`) is the best way.

bdw. what do you think about my idea about moving this out to jest test?

I actually don't mind the idea of moving this to a jest test. It's weird, I've written scripts like this so many times but I never thought about using a testing framework to report.

Does it work well?

@armano2
Copy link
Collaborator Author

armano2 commented Jan 2, 2020

it works in same way as any other test, results are same, but messages are less descriptive (jest has no concept of custom message without custom functions)

running it with other tests is actually faster (+~200ms) as opposed to ~4sec, running it in standalone mode jest tests/docs.test.ts --runTestsByPath --silent take same time.

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

This seems fine to me as is.
We can merge this and then (as discussed) convert the scripts to tests in a separate PR.

@bradzacher bradzacher changed the title chore(eslint-plugin): correct issues in documentation docs(eslint-plugin): correct issues in documentation Jan 2, 2020
@bradzacher bradzacher merged commit 3623cde into typescript-eslint:master Jan 2, 2020
@armano2 armano2 deleted the eslint-docs branch January 2, 2020 19:02
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Documentation ("docs") that needs adding/updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants