Create a script that verifies issues marked in a TODO are open

[samreid]

Last week @zepumph @chrisklus and I came across a TODO marked with an issue number and were surprised to see the issue was closed. The issue should have remained open since there was work identified in the code TODO. Would it be possible to create a script that we can run every now and then which will identify all TODOs in the code that reference issues, and for each one check the GitHub API to confirm those issues are open? For issues that have been closed, they should be reopened until the TODO is addressed.

The file chipper/eslint/rules/todo-should-have-issue.js shows how lint uses esprima to get all of the code comments for a file (includes single and multi-line comments). We would probably want to use esprima directly for simplicity: https://esprima.org/index.html

I created this issue here in chipper since it feels somewhat "lint-ish". However, I don't recommend running github API calls as part of the normal lint rules.

@ariel-phet can you please prioritize and assign?

[ariel-phet]

Marking as high priority for dev meeting - but mainly just first to decide if we should put in the effort for this script, or if this is a rare enough issue to not yet worry about.

[jessegreenberg]

Discussion 5/21/20

• We could possibly create a lint rule for this, but don't want to require linting to have a network connection for linting. So this would be an option to running lint.
• Ideally we would have a "pre-close" hook to search for todos referencing the issue you are about to close.
• SR, JB: It is worth making a quick test/pass through to see how many TODOs remain in code pointing to closed issues.
• MK: This modular task could be a good task for an intern.

Reassigning to @ariel-phet to assign someone to investigate.

[samreid]

To help determine how useful this would be, I hacked the lint script to print all TODOs with an issue reference URL (I didn't check for #NNN shorthand). Then I popped the list into a github issue comment preview, and I could mouse over each item and see which were closed. I noticed around 28. I'll reopen them and link them below so we can take a closer look.

I used this patch:

Index: eslint/rules/todo-should-have-issue.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- eslint/rules/todo-should-have-issue.js	(revision d264fd91c408e0c77cc8ab3f6f1cd15e0caa2ef6)
+++ eslint/rules/todo-should-have-issue.js	(date 1590096288407)
@@ -11,42 +11,46 @@
 module.exports = function( context ) {
 
   // Whitelist of directories to check that TODOs have GitHub issues
-  const directoriesToRequireIssues = [ /joist[/\\]js/ ];
+  // const directoriesToRequireIssues = [ /joist[/\\]js/ ];
 
   return {
 
     Program: function checkTodoShouldHaveIssue( node ) {
 
       // Check whether the given directory matches the whitelist
-      let directoryShouldBeChecked = false;
-      for ( let i = 0; i < directoriesToRequireIssues.length; i++ ) {
-        const d = directoriesToRequireIssues[ i ];
-        if ( context.getFilename().match( d ) ) {
-          directoryShouldBeChecked = true;
-          break;
-        }
-      }
+      // let directoryShouldBeChecked = true;
+      // for ( let i = 0; i < directoriesToRequireIssues.length; i++ ) {
+      //   const d = directoriesToRequireIssues[ i ];
+      //   if ( context.getFilename().match( d ) ) {
+      //     directoryShouldBeChecked = true;
+      //     break;
+      //   }
+      // }
 
-      if ( directoryShouldBeChecked ) {
+      if ( true ) {
         const comments = context.getSourceCode().getAllComments();
 
         if ( comments ) {
           for ( let i = 0; i < comments.length; i++ ) {
             const comment = comments[ i ];
 
-            if ( comment.value.indexOf( 'TODO' ) >= 0 ) {
+            if ( comment.value.indexOf( 'TODO' ) >= 0 && comment.value.indexOf( '/issues/' ) >= 0 ) {
 
+
+              const startIndex = comment.value.indexOf('https://github.com/phetsims/');
+              const tail = comment.value.substring(startIndex);
+              console.log( tail );
               // '#' followed by any number of digits
-              const missingIssueNumber = comment.value.search( /#\d+/ ) === -1;
-              const missingLink = comment.value.indexOf( 'https://github.com/phetsims/' ) === -1;
-
-              if ( missingLink && missingIssueNumber ) {
-                context.report( {
-                  node: comment,
-                  loc: comment.loc.start,
-                  message: 'TODO should have an issue: ' + comment.value
-                } );
-              }
+              // const missingIssueNumber = comment.value.search( /#\d+/ ) === -1;
+              // const missingLink = comment.value.indexOf( 'https://github.com/phetsims/' ) === -1;
+              //
+              // if ( missingLink && missingIssueNumber ) {
+              //   context.report( {
+              //     node: comment,
+              //     loc: comment.loc.start,
+              //     message: 'TODO should have an issue: ' + comment.value
+              //   } );
+              // }
             }
           }
         }

[zepumph]

Good progress here today! I made grunt reopen-issues-from-todos in perennial, and it is working well. Because of @samreid's great work here, in the previous months, there were only 8 issues that needed to be reopened. Next step is adding to daily grunt work and then code review. Commits to come soon.

[zepumph]

Ok. That should do it. @samreid can you please review?

[zepumph]

Here is the log from daily-grunt-work last night:

REOPEN ISSUES LINKED IN TODOS:
Running "reopen-issues-from-todos" task
grunt lint-everything started
grunt lint-everything finished
118 issues to check

When it catches something for reopening, it would say something like this for each issue below those lines: TODO linking to closed issue: ISSUE

[samreid]

Looks very great! Nice work. Some review comments:

• If this is the first script someone runs before chipper/dist exists, it will have problems writing that file
• Some entries in issuesFromTODOs.txt end with ),. characters. May cause problems.
• I ran it and it correctly detected AccordionBox layout (info button and dot plot) center-and-variability#166 (comment), awesome!
• I did not take the time to fully understand each regex

[zepumph]

Noting that we can see all the issues that have been effected by this script with:

https://github.com/issues?q=is%3Aopen+is%3Aissue+user%3Aphetsims+%22Reopening+because+there+is+a+TODO+marked+for+this+issue.%22

I updated to support if chipper/dist doesn't exist.

I see that my regex doesn't support something like this: https://github.com/phetsims/aqua/blob/d9bf6eba1244186646af8d236bcb5bfccbedd2a1/js/local/unitTestBatch.js#L32. Updating now.

[zepumph]

Anything else here?

[samreid]

Excellent, the changes seem good. Thanks for building this, it has already been very useful! Closing.