Addition of eslint plugin #190

bafolts · 2019-10-25T01:48:58Z

This pull request adds an eslint plugin which validates tsdoc comment blocks. This should resolve #154

msftclas · 2019-10-25T01:49:12Z

All CLA requirements met.

iclanton · 2019-10-28T17:41:09Z

eslint-plugin/.npmrc

@@ -0,0 +1,2 @@
+registry=https://registry.npmjs.org/


You shouldn't need this file.

If so we should also delete the copy under /tsdoc/.npmrc.

I think you're right though -- this probably predates the ADO publishing pipeline.

This probably actually predates this repo's migration to Rush. Let's clean these up in a follow-up PR.

iclanton · 2019-10-28T17:41:58Z

eslint-plugin/package.json

+  "private": true,
+  "license": "MIT",
+  "scripts": {
+    "build": "",


You definitely want a CI build script here.

iclanton · 2019-10-28T17:42:45Z

eslint-plugin/package.json

+  "name": "eslint-plugin-tsdoc",
+  "version": "0.1.0",
+  "description": "eslint plugin for tsdoc",
+  "private": true,


We expect to publish this plugin, no?

iclanton · 2019-10-28T17:43:02Z

eslint-plugin/src/index.js

@@ -0,0 +1,44 @@
+


This should be written in TypeScript.

iclanton · 2019-10-28T17:43:42Z

eslint-plugin/tests/index.js

@@ -0,0 +1,14 @@
+


Our convention is to put tests under src/<relevant path>/tests/<featurename>.test.ts.

This should also be written in TypeScript.

iclanton · 2019-10-28T17:50:14Z

eslint-plugin/src/index.js

+                            var commentString = "/*" + commentBlock.value + "*/";
+                            var results = tsDocParser.parseString(commentString).log;
+                            if (results._messages.length > 0) {
+                                results._messages.forEach(function (message) {


It'd be more efficient to use a for(... of ...) loop here.

iclanton · 2019-10-28T17:51:12Z

eslint-plugin/src/index.js

+                            var commentBlock = commentBlocks[0];
+                            var commentString = "/*" + commentBlock.value + "*/";
+                            var results = tsDocParser.parseString(commentString).log;
+                            if (results._messages.length > 0) {


You actually don't need this check. If there are no entries in the _messages property, the loop won't iterate.

iclanton · 2019-10-28T17:51:46Z

eslint-plugin/src/index.js

+                            var commentString = "/*" + commentBlock.value + "*/";
+                            var results = tsDocParser.parseString(commentString).log;
+                            if (results._messages.length > 0) {
+                                results._messages.forEach(function (message) {


Properties that begin with underscores (like _messages) are private. Use a public property here.

iclanton · 2019-10-28T17:55:00Z

eslint-plugin/src/index.js

+                            var results = tsDocParser.parseString(commentString).log;
+                            if (results._messages.length > 0) {
+                                results._messages.forEach(function (message) {
+                                    context.report({


This will report every TSDoc parse error as a separate linting violation, won't it? Is that what we want? Or do we want to report a single violation that says that the doc comment is invalid, and list the parse errors as additional information on that single violation?

I think we do want separate lint errors for each TSDoc error. This way the red squiggly underlines can be as accurate as possible.

iclanton · 2019-10-28T18:01:13Z

eslint-plugin/src/index.js

+module.exports = {
+    rules: {
+        "tsdoc-comments": {
+            meta: {


We should include at least the type and docs properties here (https://github.com/eslint/eslint/blob/master/docs/developer-guide/working-with-rules.md#rule-basics).

bafolts · 2019-10-29T03:08:31Z

I will make the suggested changes here and comment back when I am done. Thanks for the feedback.

bafolts · 2019-10-31T13:27:59Z

I believe I have made all the suggested improvements. Let me know if there are any more.

iclanton · 2019-10-31T23:00:42Z

eslint-plugin/package.json

+  "private": false,
+  "license": "MIT",
+  "scripts": {
+    "build": "tsc",


We probably want to run eslint on this package as well. Maybe copy parts of the build script from tsdoc.

iclanton · 2019-10-31T23:01:13Z

eslint-plugin/src/index.ts

+});
+
+module.exports = {
+    rules: {


Can you use two-space indentation?

iclanton · 2019-10-31T23:01:24Z

eslint-plugin/src/index.ts

+    messageIds[messageId] = messageId + ": {{ unformattedText }}";
+});
+
+module.exports = {


You should just be able to say exports = ....

Suggested change

module.exports = {

exports = {

iclanton · 2019-10-31T23:02:07Z

eslint-plugin/src/index.ts

+const messageIds: {[x: string]: string} = {};
+
+allTsdocMessageIds.forEach((messageId: string) => {
+    messageIds[messageId] = messageId + ": {{ unformattedText }}";


You can use a template string here:

Suggested change

messageIds[messageId] = messageId + ": {{ unformattedText }}";

messageIds[messageId] = `${messageId}: {{ unformattedText }}`;

iclanton · 2019-10-31T23:02:35Z

eslint-plugin/src/index.ts

+                    url: "https://github.com/microsoft/tsdoc"
+                }
+            },
+            create: function(context: eslint.Rule.RuleContext) {


Use a lambda function here.

Suggested change

create: function(context: eslint.Rule.RuleContext) {

create: (context: eslint.Rule.RuleContext) => {

iclanton · 2019-10-31T23:07:35Z

eslint-plugin/src/index.ts

+                        var commentBlock = commentBlocks[0];
+                        var commentString = "/*" + commentBlock.value + "*/";
+                        var results = tsDocParser.parseString(commentString).log;
+                        for (let message of results.messages) {


Suggested change

for (let message of results.messages) {

for (const message of results.messages) {

iclanton · 2019-10-31T23:09:04Z

eslint-plugin/src/tests/index.ts

@@ -0,0 +1,26 @@
+
+import { RuleTester } from "eslint";
+const rule = require("../index").rules["tsdoc-comments"];


Suggested change

const rule = require("../index").rules["tsdoc-comments"];

import * as rules from '../index';

const rule: eslint.Rule /* or whatever the type is here */ = rules['tsdoc-comments'];

iclanton · 2019-10-31T23:10:50Z

eslint-plugin/package.json

+  "license": "MIT",
+  "scripts": {
+    "build": "tsc",
+    "test": "node tests/index.js"


This path should now be lib/tests/index.js.

Although it'd probably be best to include this in the build script so we run the test as part of the build.

Suggested change

"test": "node tests/index.js"

"test": "node lib/tests/index.js"

iclanton · 2019-10-31T23:11:23Z

eslint-plugin/package.json

+    "@microsoft/tsdoc": "0.12.14"
+  },
+  "devDependencies": {
+    "@types/eslint": "^6.1.3",


@types/ packages don't follow semver. Pin this to the specific version.

Suggested change

"@types/eslint": "^6.1.3",

"@types/eslint": "6.1.3",

iclanton · 2019-10-31T23:13:50Z

eslint-plugin/src/index.ts

+    messageIds[messageId] = messageId + ": {{ unformattedText }}";
+});
+
+module.exports = {


It would be useful to ensure that the object we export is well-typed.

iclanton · 2019-11-01T04:37:55Z

Thanks so much for putting this together! This rule will be really useful.

Most of the remaining comments are fairly superficial, and they can probably be addressed in a follow-up PR if you like.

bafolts · 2019-11-03T00:08:16Z

I have made all the requested changes. It appears something with the build is failing in the playground directory. If I update the @types/react-dom version to 16.9.3 locally I can fix the problem, but for the life of me I can't get rushstack to build with the version I specify in playground/package.json.

octogonz · 2019-11-03T01:06:38Z

It's this old issue. The TSDoc repo was serving as a guinea pig for the Yarn package manager, but Yarn really doesn't play well with DefinitelyTyped. There's an active megathread where they're finally trying to sort this out. @iclanton I wonder if we should just switch to PNPM for the time being?

I've pushed a fix to your branch.

iclanton · 2019-11-03T09:04:54Z

eslint-plugin/.npmrc

@@ -0,0 +1,2 @@
+registry=https://registry.npmjs.org/


This probably actually predates this repo's migration to Rush. Let's clean these up in a follow-up PR.

bafolts requested review from iclanton and octogonz as code owners October 25, 2019 01:48

iclanton requested changes Oct 28, 2019

View reviewed changes

bafolts force-pushed the eslint-plugin branch from bedc828 to 21ac1fa Compare October 31, 2019 13:17

bafolts requested a review from iclanton October 31, 2019 13:28

bafolts force-pushed the eslint-plugin branch from 21ac1fa to 00d8f0f Compare October 31, 2019 13:28

iclanton requested changes Oct 31, 2019

View reviewed changes

bafolts force-pushed the eslint-plugin branch 3 times, most recently from 1aaf298 to 3ac9f2f Compare November 2, 2019 23:34

Addition of eslint-plugin

692ad88

bafolts force-pushed the eslint-plugin branch from 3ac9f2f to 692ad88 Compare November 3, 2019 00:10

octogonz added 2 commits November 2, 2019 17:57

Resync versions

fa2d838

rush update --full --purge

32b9e1b

Deleted package-lock.json

fc16343

iclanton approved these changes Nov 3, 2019

View reviewed changes

iclanton merged commit 3f3dc73 into microsoft:master Nov 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addition of eslint plugin #190

Addition of eslint plugin #190

bafolts commented Oct 25, 2019

msftclas commented Oct 25, 2019 •

edited

Loading

iclanton Oct 28, 2019

octogonz Oct 28, 2019

iclanton Nov 3, 2019

iclanton Oct 28, 2019

iclanton Oct 28, 2019

iclanton Oct 28, 2019

iclanton Oct 28, 2019

iclanton Oct 28, 2019

iclanton Oct 28, 2019

iclanton Oct 28, 2019

iclanton Oct 28, 2019

octogonz Oct 28, 2019

iclanton Oct 28, 2019

bafolts commented Oct 29, 2019

bafolts commented Oct 31, 2019

iclanton Oct 31, 2019

iclanton Oct 31, 2019

iclanton Oct 31, 2019 •

edited

Loading

iclanton Oct 31, 2019

iclanton Oct 31, 2019

iclanton Oct 31, 2019

iclanton Oct 31, 2019

iclanton Oct 31, 2019 •

edited

Loading

iclanton Oct 31, 2019 •

edited

Loading

iclanton Oct 31, 2019

iclanton commented Nov 1, 2019

bafolts commented Nov 3, 2019

octogonz commented Nov 3, 2019

iclanton Nov 3, 2019

	messageIds[messageId] = messageId + ": {{ unformattedText }}";
	messageIds[messageId] = `${messageId}: {{ unformattedText }}`;

	create: function(context: eslint.Rule.RuleContext) {
	create: (context: eslint.Rule.RuleContext) => {

	for (let message of results.messages) {
	for (const message of results.messages) {

		@@ -0,0 +1,26 @@

		import { RuleTester } from "eslint";
		const rule = require("../index").rules["tsdoc-comments"];

	const rule = require("../index").rules["tsdoc-comments"];
	import * as rules from '../index';
	const rule: eslint.Rule /* or whatever the type is here */ = rules['tsdoc-comments'];

	"test": "node tests/index.js"
	"test": "node lib/tests/index.js"

Addition of eslint plugin #190

Addition of eslint plugin #190

Conversation

bafolts commented Oct 25, 2019

msftclas commented Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bafolts commented Oct 29, 2019

bafolts commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

iclanton Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton commented Nov 1, 2019

bafolts commented Nov 3, 2019

octogonz commented Nov 3, 2019

Choose a reason for hiding this comment

msftclas commented Oct 25, 2019 •

edited

Loading

iclanton Oct 31, 2019 •

edited

Loading

iclanton Oct 31, 2019 •

edited

Loading

iclanton Oct 31, 2019 •

edited

Loading