Merge pull request microsoft#1307 from microsoft/ianc/improved-change…

…-error

[rush] Improve messaging around "rush change"

apps/rush-lib/src/cli/actions/ChangeAction.ts

@@ -96,22 +96,22 @@ export class ChangeAction extends BaseRushAction {
       parameterLongName: '--target-branch',
       parameterShortName: '-b',
       argumentName: 'BRANCH',
-      description: 'If this parameter is specified, compare current branch with the target branch to get changes. ' +
-        'If this parameter is not specified, the current branch is compared against the "master" branch.'
+      description: 'If this parameter is specified, compare the checked out branch with the specified branch to' +
+        'determine which projects were changed. If this parameter is not specified, the checked out branch ' +
+        'is compared against the "master" branch.'
     });
   }
 
   public run(): Promise<void> {
-    console.log(`Target branch is ${this._targetBranch}`);
+    console.log(`The target branch is ${this._targetBranch}`);
     this._projectHostMap = this._generateHostMap();
 
     if (this._verifyParameter.value) {
       this._verify();
       return Promise.resolve();
     }
-    this._sortedProjectList = this._getChangedPackageNames()
-      .sort();
 
+    this._sortedProjectList = this._getChangedPackageNames().sort();
     if (this._sortedProjectList.length === 0) {
       console.log('No changes were detected on this branch. Nothing to do.');
       this._warnUncommittedChanges();
@@ -124,7 +124,7 @@ export class ChangeAction extends BaseRushAction {
 
     return this._promptLoop()
       .catch((error: Error) => {
-        throw new Error(`There was an error creating the change file: ${error.toString()}`);
+        throw new Error(`There was an error creating a change file: ${error.toString()}`);
       });
   }
 
@@ -153,8 +153,9 @@ export class ChangeAction extends BaseRushAction {
 
   private get _targetBranch(): string {
     if (!this._targetBranchName) {
-      this._targetBranchName = this._targetBranchParameter.value ||
-        VersionControl.getRemoteMasterBranch(this.rushConfiguration.repositoryUrl);
+      this._targetBranchName = (
+        this._targetBranchParameter.value || VersionControl.getRemoteMasterBranch(this.rushConfiguration.repositoryUrl)
+      );
     }
 
     return this._targetBranchName;
@@ -184,9 +185,6 @@ export class ChangeAction extends BaseRushAction {
 
   private _validateChangeFile(changedPackages: string[]): void {
     const files: string[] = this._getChangeFiles();
-    if (files.length === 0) {
-      throw new Error(`Your branch has changes that require a change log entry. Please run "rush change".`);
-    }
     ChangeFiles.validate(files, changedPackages);
   }
 
@@ -221,7 +219,7 @@ export class ChangeAction extends BaseRushAction {
       return this._askQuestions(this._sortedProjectList.pop()!)
         .then((answers: IChangeInfo) => {
           if (answers) {
-            // Save the info into the changefile
+            // Save the info into the change file
             let changeFile: IChangeFile | undefined = this._changeFileData.get(answers.packageName);
             if (!changeFile) {
               changeFile = {
@@ -417,24 +415,27 @@ export class ChangeAction extends BaseRushAction {
         }
       }
     ])
-      .then(({ email }) => {
-        return email;
-      });
+    .then(({ email }) => email);
   }
 
   private _warnUncommittedChanges(): void {
     try {
       if (VersionControl.hasUncommittedChanges()) {
-        console.log(os.EOL +
-          colors.yellow('Warning: You have uncommitted changes, which do not trigger a change entry.'));
+        console.log(
+          os.EOL +
+          colors.yellow(
+            'Warning: You have uncommitted changes, which do not trigger prompting for change ' +
+            'descriptions.'
+          )
+        );
       }
     } catch (error) {
-      console.log('Ignore the failure of checking uncommitted changes');
+      console.log(`An error occurred when detecting uncommitted changes: ${error}`);
     }
   }
 
   /**
-   * Writes changefile to the common/changes folder. Will prompt for overwrite if file already exists.
+   * Writes change files to the common/changes folder. Will prompt for overwrite if file already exists.
    */
   private _writeChangeFiles(): void {
     this._changeFileData.forEach((changeFile: IChangeFile) => {
@@ -455,12 +456,11 @@ export class ChangeAction extends BaseRushAction {
           type: 'confirm',
           message: `Overwrite ${filePath} ?`
         }
-      ]).then(({ overwrite }: { overwrite: string }) => {
+      ]).then(({ overwrite }) => {
         if (overwrite) {
-          return this._writeFile(filePath, output);
+          this._writeFile(filePath, output);
         } else {
           console.log(`Not overwriting ${filePath}...`);
-          return Promise.resolve();
         }
       }).catch((error) => {
         console.error(colors.red(error.message));
@@ -474,9 +474,7 @@ export class ChangeAction extends BaseRushAction {
    * Writes a file to disk, ensuring the directory structure up to that point exists
    */
   private _writeFile(fileName: string, output: string): void {
-    FileSystem.writeFile(fileName, output, {
-      ensureFolderExists: true
-    });
-    console.log('Created file: ' + fileName);
+    FileSystem.writeFile(fileName, output, { ensureFolderExists: true });
+    console.log(`Created file: ${fileName}`);
   }
 }

apps/rush-lib/src/cli/test/__snapshots__/CommandLineHelp.test.ts.snap

@@ -166,10 +166,11 @@ Optional arguments:
   --no-fetch            Skips fetching the baseline branch before running 
                         \\"git diff\\" to detect changes.
   -b BRANCH, --target-branch BRANCH
-                        If this parameter is specified, compare current 
-                        branch with the target branch to get changes. If this 
-                        parameter is not specified, the current branch is 
-                        compared against the \\"master\\" branch.
+                        If this parameter is specified, compare the checked 
+                        out branch with the specified branch todetermine 
+                        which projects were changed. If this parameter is not 
+                        specified, the checked out branch is compared against 
+                        the \\"master\\" branch.
 "
 `;
 

apps/rush-lib/src/logic/ChangeFiles.ts

@@ -28,30 +28,30 @@ export class ChangeFiles {
     newChangeFilePaths: string[],
     changedPackages: string[]
   ): void {
-    const changedSet: Set<string> = new Set<string>();
+    const projectsWithChangeDescriptions: Set<string> = new Set<string>();
     newChangeFilePaths.forEach((filePath) => {
       console.log(`Found change file: ${filePath}`);
 
-      const changeRequest: IChangeInfo = JsonFile.load(filePath);
-      if (changeRequest && changeRequest.changes) {
-        changeRequest.changes!.forEach(change => {
-          changedSet.add(change.packageName);
-        });
+      const changeFile: IChangeInfo = JsonFile.load(filePath);
+      if (changeFile && changeFile.changes) {
+        changeFile.changes.forEach(change => projectsWithChangeDescriptions.add(change.packageName));
       } else {
         throw new Error(`Invalid change file: ${filePath}`);
       }
     });
 
-    const requiredSet: Set<string> = new Set(changedPackages);
-    changedSet.forEach((name) => {
-      requiredSet.delete(name);
-    });
-    if (requiredSet.size > 0) {
-      const missingProjects: string[] = [];
-      requiredSet.forEach(name => {
-        missingProjects.push(name);
-      });
-      throw new Error(`Change file does not contain ${missingProjects.join(',')}.`);
+    const projectsMissingChangeDescriptions: Set<string> = new Set(changedPackages);
+    projectsWithChangeDescriptions.forEach((name) => projectsMissingChangeDescriptions.delete(name));
+    if (projectsMissingChangeDescriptions.size > 0) {
+      const projectsMissingChangeDescriptionsArray: string[] = [];
+      projectsMissingChangeDescriptions.forEach(name => projectsMissingChangeDescriptionsArray.push(name));
+      throw new Error([
+        'The following projects have been changed and require change descriptions, but change descriptions were not ' +
+          'detected for them:',
+        ...projectsMissingChangeDescriptionsArray.map((projectName) => `- ${projectName}`),
+        'To resolve this error, run "rush change." This will generate change description files that must be ' +
+          'committed to source control.'
+      ].join(EOL));
     }
   }
 

common/changes/@microsoft/rush/ianc-improved-change-error_2019-05-30-21-23.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "comment": "Clarify \"rush change\" messages.",
+      "packageName": "@microsoft/rush",
+      "type": "none"
+    }
+  ],
+  "packageName": "@microsoft/rush",
+  "email": "[email protected]"
+}