feat: print clang-format output #186

lee-tammy · 2018-07-10T01:00:07Z

Still working on the feature so that it can support non-utf8 characters.

Fixes #181

googlebot · 2018-07-10T01:00:11Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-07-10T02:18:41Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

lee-tammy · 2018-07-10T02:20:21Z

Should work on files with non-utf8 characters now

…bytes

googlebot · 2018-07-10T17:38:07Z

CLAs look good, thanks!

kjin

Algorithm generally looks good! Left some comments.

kjin · 2018-07-10T18:14:57Z

package.json

    "inquirer": "^6.0.0",
    "meow": "^5.0.0",
    "pify": "^3.0.0",
    "rimraf": "^2.6.2",
    "tslint": "^5.9.1",
    "update-notifier": "^2.5.0",
-    "write-file-atomic": "^2.3.0"
+    "utfstring": "^2.0.0",


I think utfstring and xml2js are no longer used?

After you remove them, you can run npm install again to effect changes in package-lock.json.

kjin · 2018-07-10T18:17:06Z

src/format.ts

@@ -15,11 +15,18 @@
 */
 import * as fs from 'fs';
 import * as path from 'path';
+import {promisify} from 'util';


I believe promisify isn't available in Node 6, which is the minimum version we support.

kjin · 2018-07-10T18:17:43Z

src/format.ts

@@ -38,7 +45,7 @@ export async function format(
    fix = false;
  }

-  // If the project has a .clang-format – use it. Else use the default as an
+  // If the project has a .clang-format i– use it. Else use the default as an


Was this an accidental change?

yes, I changed it back

kjin · 2018-07-10T18:22:36Z

src/format.ts

+
+    if (formatErr.length === null || formatErr.offset === null ||
+        formatErr.fix === null) {
+      return;


I would suggest printing a message here on stderr to suggest that clang-format didn't emit expected output before returning. Also, maybe it would make more sense to continue here rather than returning?

So, instead of checking if the variables are null, should I check if they are not null and using continue ...and have an else block with the print statement that tells the user that there are no formatting issues? The only case that I can think of where formatErr.length, offset, and fix would be null, is if there are no formatting issues.

if (formatErr.length !== null && formatErr.offset !== null && formatErr.fix !== null) { continue; }else{ options.logger.log('clang-format reported no errors') }

Take a look at the changes I made so far. I'm returning an empty string[] instead of undefined. I don't think it's necessary to print to stderr because it is not an error when formatErr.length/offset/fix are null. it means that the format of the file is fine and does not need to be changed.

The only case that I can think of where formatErr.length, offset, and fix would be null, is if there are no formatting issues.

Yep, I forgot that it's possible for there to not be replacements at all, indicating no formatting issues. For handling this, you can consider continue-ing earlier, even before the regexes are being executed, by checking whether the replacement tag occurs at all. That way, when you get to the point where formatErr.length === null || formatErr.offset === null || formatErr.fix === null is being evaluated, you know that if that statement is true, it indicates that clang-format didn't emit expected output. Even though I wouldn't expect clang-format to change its output xml format in the future, this would help us disambiguate whether we are continuing because there are no formatting issues, or because an unexpected change occurred in clang-format's output.

Ok, I added an if statement with continue before calling the getReplacement method that uses regex. Also, I made it so that it throws an error if the regex results for offset, length, and fixes arrays are null or if their lengths are not the same.

kjin · 2018-07-10T18:24:33Z

src/format.ts

+      formatErr.fix[j] = entities.decodeXML(formatErr.fix[j]);
+    }
+
+    const read = promisify(fs.readFile);


If I'm not mistaken, I think the promisified readFile is already available in utils.ts.

kjin · 2018-07-10T20:30:19Z

src/format.ts

+  replaced.push(replacements[errOffset.length - 1]);
+  replaced.push(substring(
+      data, +errOffset[errOffset.length - 1] + +errLength[errOffset.length - 1],
+      Buffer.byteLength(data, 'utf8')));


kjin · 2018-07-10T20:32:24Z

src/format.ts

+ * @param diffs contains all information about the formatting changes
+ * @param options
+ */
+function printDiffs(file: string, diffs: any, options: Options) {


I believe that if you import from diff, then you can get the interface that matches the any here.

kjin · 2018-07-10T20:33:59Z

src/format.ts

 import {Options} from './cli';
 import {createProgram} from './lint';

 // Exported for testing purposes.
 export const clangFormat = require('clang-format');
+const xml2js = require('xml2js');
+const chalk = require('chalk');


Can you use import * as x from 'x' instead of const x = require('x')? TS will prompt you for type definitions -- you can get them with

npm install --save-dev @types/x

The @types/ packages contain type definitions for each package. Excluding xml and utf (because you're not using them), I believe the rest of them all have this associated package.

kjin · 2018-07-10T20:34:30Z

src/format.ts

+/**
+ * Substring by bytes
+ * 
+ * @param str 


Can you fill in the parameter decsriptions here?

kjin · 2018-07-10T20:34:57Z

src/format.ts

+  return Buffer.from(str, encoding)
+      .slice(indexStart, indexEnd)
+      .toString(encoding);
+}


Can you add a new line to the end of this file? (It's our general practice to do so)

…s passed

ofrobots · 2018-07-11T23:11:53Z

package.json

@@ -42,6 +42,7 @@
  "dependencies": {
    "chalk": "^2.4.1",
    "clang-format": "1.2.3",
+    "entities": "^1.1.1",


Did you miss diff here?

yes, added it back

ofrobots · 2018-07-11T23:18:02Z

src/format.ts

+      file, 'no new file', text, fixed, 'no old header', 'no new header',
+      {context: 3});
+  jsdiff.applyPatch('diff', diff);
+  return Promise.resolve(diff);


Change this to simply return diff. Async functions automatically wrap return values in a promise.

ofrobots · 2018-07-11T23:21:00Z

src/format.ts

+ */
+function substring(
+    str: string, indexStart: number, indexEnd: number, encoding = 'utf8') {
+  return Buffer.from(str, encoding)


This looks odd. Since we already had a string – we already had decoded it using whatever encoding that was necessary. Why are we converting to a buffer and then converting back to a string? This does extra work in encoding/decoding, and will be buggy in cases where a different encoding ought to have been used (as rare as it might be).

There isn't an available bytesSubstring function out there. I was thinking that we should publish this as separate module (it does in fact have subtle differences from substring).

Given that this function is only used for showing output, and that JS files are generally written with UTF-8 encoding, I feel like it's acceptable to say (for now) that if your file is encoded differently, you're on your own. That being said, any static tool that prints code snippets is going to have to do the same operation somewhere, so maybe we should look to tslint or others for inspiration. I wouldn't be too surprised if tslint makes the UTF-8 assumption as well.

Discovered this: palantir/tslint#2810

ofrobots · 2018-07-11T23:23:14Z

I just tried this out. This looks really cool!

kjin · 2018-07-11T23:50:21Z

src/format.ts

-import { start } from 'repl';
+import {readFilep} from './util';
+
+interface replacement {


Interfaces names should be capitalized. Also, could you add a JSDoc describing the role of this object? (By JSDoc I mean /** */ style rather than // style)

kjin · 2018-07-11T23:51:31Z

src/format.ts

@@ -100,11 +101,11 @@ function fixFormat(srcFiles: string[], baseArgs: string[]): Promise<boolean> {
 *
 * @param srcFiles list of source files
 */
-function checkFormat(srcFiles: string[], baseArgs: string[], options: Options): Promise<boolean> {
+async function checkFormat(


I believe this function does not need to be async, since the use of await is in a nested function.

oops, forgot to remove

kjin · 2018-07-11T23:52:39Z

src/format.ts

+
+    out.on('end', async () => {
+      const files = output.split('<?xml version=\'1.0\'?>\n');
+      for (let i = 1; i < files.length; i++) {


I would recommend using slice so that we can just start this loop at i = 0.

kjin · 2018-07-11T23:53:17Z

src/format.ts

+      for (let i = 1; i < files.length; i++) {
+        const replacements = getReplacements(files[i]);
+        if (replacements.length > 0) {
+          const diff = await getDiffObj(srcFiles[i - 1], replacements)


This line seems to be missing a ;? Maybe you need to run npm run fix

kjin · 2018-07-11T23:55:37Z

src/format.ts

-  for(let i = 1; i < files.length; i++){
+function getReplacements(output: string): replacement[] {
+  const replacements: replacement[] = [];
+  const offsets: string[]|null = output.match(/(?<=offset=\')(\d+)(?=\')/g);


I think the approach in this function would benefit from an inline comment.

done. Check to see if it's ok

kjin · 2018-07-11T23:58:34Z

src/format.ts

+    const fixed =
+        performFixes(text, formatErr.offset, formatErr.length, formatErr.fix);
+    const diff = jsdiff.structuredPatch(
+        'oldFile', 'newFile', text, fixed, 'old', 'new', {context: 3});


You can probably just assign empty strings, if you aren't reading them elsewhere.

codecov-io · 2018-07-12T20:58:45Z

Codecov Report

Merging #186 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   98.15%   98.37%   +0.21%     
==========================================
  Files          12       12              
  Lines         652      737      +85     
  Branches       55       61       +6     
==========================================
+ Hits          640      725      +85     
  Misses         12       12

Impacted Files	Coverage Δ
test/test-format.ts	`100% <100%> (ø)`	⬆️
src/format.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e107ab3...2b80caa. Read the comment docs.

kjin · 2018-07-13T16:42:00Z

cc/ @jplaisted

kjin

Almost there!

kjin · 2018-07-13T20:02:42Z

src/format.ts

@@ -92,7 +107,8 @@ function fixFormat(srcFiles: string[], baseArgs: string[]): Promise<boolean> {
 *
 * @param srcFiles list of source files
 */
-function checkFormat(srcFiles: string[], baseArgs: string[]): Promise<boolean> {
+function checkFormat(srcFiles: string[], baseArgs: string[], options: Options):


Nit: Can we emulate format and put the options argument first?

kjin · 2018-07-13T20:06:43Z

src/format.ts

+
+  let xmlLines = fileXML.split('\n');
+  // The first and last two elements in xmlLines are not needed
+  xmlLines = xmlLines.slice(1, xmlLines.length - 2);


Suggestion: It wasn't immediately obvious to me that xmlLines.length - 2 is correct. If you define xmlLines as fileXML.trim().split('\n') then it will guarantee one element per non-empty line, so you could probably use xmlLines.length - 1.

kjin · 2018-07-13T20:07:14Z

src/format.ts

+
+  for (let i = 0; i < xmlLines.length; i++) {
+    // Uses regex to capture the xml attribute and element
+    // XML format: <replacement offset='OFFSET' length='LENGTH'>FIX</replacement


Nit: Missing ending >

kjin · 2018-07-13T20:08:47Z

src/format.ts

+ * @param diffs contains all information about the formatting changes
+ * @param options
+ */
+function printDiffs(diffs: jsdiff.IUniDiff, options: Options) {


Suggestion: For string constructions in this file, try string interpolation.

const x = 2; console.log(`${x + x} = 4`) // prints "4 = 4"

I'm using string interpolation now. Check if it's okay

kjin · 2018-07-13T20:10:38Z

test/test-format.ts

 import * as format from '../src/format';
 import {nop} from '../src/util';

 import {withFixtures} from './fixtures';

 // clang-format won't pass this code because of trailing spaces.
-const BAD_CODE = 'export const foo = [ 2 ];';
-const GOOD_CODE = 'export const foo = [2];';
+const BAD_CODE = '//🦄\nexport const foo = [ \"2\" ];';


As we discussed in person, let's have the unicorn be in its own test.

kjin · 2018-07-16T16:54:43Z

src/format.ts


  for (let i = 0; i < xmlLines.length; i++) {
    // Uses regex to capture the xml attribute and element
-    // XML format: <replacement offset='OFFSET' length='LENGTH'>FIX</replacement
+    // XML format: <replacement offset='OFFSET'


This looks like it was "fixed" with npm run fix. Can you put the <...> on its own line so it doesn't get broken up?

kjin

~~LGTM 👍~~

Tests don't pass

kjin

LGTM, and actually tests do pass!

kjin

LGTM 👍

googlebot added the cla: no label Jul 10, 2018

kjin self-requested a review July 10, 2018 01:00

lee-tammy changed the title ~~Print clang-format output feature~~ feat: print clang-format output Jul 10, 2018

lee-tammy added 8 commits July 10, 2018 10:31

Started working using regex

3bdc619

Working on apply patches in jsdiff

ab8bc97

Able to separate string into multiple diffs and print them

0fa2476

Added comments and changed formatting -- Has special character bug

314e3ca

Changed output formatting -- does not support utf8 characters

d692f24

Revert changes in a comment

7f34080

Changed original substring function to special substring that counts …

bb5f697

…bytes

Added dependencies

22933d6

lee-tammy force-pushed the print-format-lines-v3 branch from 5a29ea8 to 22933d6 Compare July 10, 2018 17:38

googlebot added cla: yes and removed cla: no labels Jul 10, 2018

kjin reviewed Jul 10, 2018

View reviewed changes

lee-tammy added 2 commits July 10, 2018 18:59

Made some changes

36d0299

Split functions and added comments -- possible error when directory i…

13a78ad

…s passed

ofrobots reviewed Jul 11, 2018

View reviewed changes

Forgot to add diff dependency

ff94ab0

kjin reviewed Jul 11, 2018

View reviewed changes

lee-tammy added 2 commits July 11, 2018 18:50

Added some comments and added some error checks

e4d1945

Updated regex -- should work on node 6 now

5c6f5f5

lee-tammy and others added 3 commits July 13, 2018 09:43

Added a test for formating

7844b05

Commit for merge

b46b96e

Merged conflicts

b7f7c31

kjin reviewed Jul 13, 2018

View reviewed changes

Added tests and made minor changes

850a455

kjin reviewed Jul 16, 2018

View reviewed changes

Fixed comment

9b4521a

kjin previously approved these changes Jul 16, 2018

View reviewed changes

Used lint and format

8f02bce

kjin approved these changes Jul 16, 2018

View reviewed changes

lee-tammy added 5 commits July 16, 2018 15:26

Changed existing tests

a37c01b

Removed Logger import statement b/c it is not used anymore

ddd5d81

Fixed tests and added one to check for error

d8575d4

Fixed line over 80 chars

bc47938

forgot to run lint

b57c458

kjin approved these changes Jul 17, 2018

View reviewed changes

kjin added 2 commits July 16, 2018 17:42

Merge branch 'master' into print-format-lines-v3

69b3211

Merge branch 'master' into print-format-lines-v3

2b80caa

kjin merged commit f7c75ee into google:master Jul 17, 2018

feat: print clang-format output #186

feat: print clang-format output #186

Conversation

lee-tammy commented Jul 10, 2018 • edited by kjin

googlebot commented Jul 10, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Jul 10, 2018

lee-tammy commented Jul 10, 2018

googlebot commented Jul 10, 2018

kjin left a 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

lee-tammy Jul 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lee-tammy Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lee-tammy Jul 12, 2018 • edited

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

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

ofrobots commented Jul 11, 2018

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

codecov-io commented Jul 12, 2018 • edited

Codecov Report

kjin commented Jul 13, 2018

kjin left a 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

kjin left a comment • edited

Choose a reason for hiding this comment

kjin left a comment

Choose a reason for hiding this comment

kjin left a comment • edited

Choose a reason for hiding this comment

lee-tammy commented Jul 10, 2018 •

edited by kjin

lee-tammy Jul 10, 2018 •

edited

lee-tammy Jul 11, 2018 •

edited

lee-tammy Jul 12, 2018 •

edited

codecov-io commented Jul 12, 2018 •

edited

kjin left a comment •

edited

kjin left a comment •

edited