Skip to content
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

feat: allow globbing #23

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ If your platform isn't yet supported, you can create the native binary from
the latest upstream clang sources, make sure it is stripped and optimized
(should be about 1.4MB as of mid-2015) and send a pull request to add it.

## Globbing files

$ clang-format --glob=folder/**/*.js

This will run `clang-format` once per file result, and show the total
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to document what the globbing semantics are. It's sufficient to point to the docs for the glob library we use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger, will add this.

formatted files at the end.
See [node-glob](https://github.com/isaacs/node-glob) for globbing semantics.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized that we don't afford a way to pass options - I was just trying to write up an example and need exclude...


## Compiling clang-format

For Linux, compile a statically linked MinSizeRel build:
Expand Down
53 changes: 48 additions & 5 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ var os = require('os');
var path = require('path');
var resolve = require('resolve').sync;
var spawn = require('child_process').spawn;
var glob = require("glob");
var async = require("async");
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a funny name for it now. Is the library worth having the dependency now, rather than just a for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the operation itself (spawning a child process and waiting for it to end) is still async so a simple for loop wouldn't work.

I could replicate async's series functionality with a well crafted recursive function, but async does it better and is more well tested so it seemed a better fit for this wrapper.


var VERSION = require('./package.json').version;
var LOCATION = __filename;
Expand Down Expand Up @@ -48,11 +50,52 @@ function spawnClangFormat(args, done, stdio) {
"Consider installing it with your native package manager instead.\n";
throw new Error(message);
}
var clangFormatProcess = spawn(nativeBinary, args, {stdio: stdio});
clangFormatProcess.on('close', function(exit) {
if (exit) done(exit);
});
return clangFormatProcess;

// extract glob, if present
var filesGlob = args.filter(function(arg){return arg.indexOf('--glob=') === 0;})
.map(function(arg){return arg.replace('--glob=', '');})
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure I understand why you replace with empty string here, but then also filter it out at line 61

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I replace it in a copy of the array to just get the glob itself. Below I remove the glob arg from the args array itself, because otherwise clang would receive it and thus error.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I see, it's because the glob pattern is part of the same arg string with the flag

.shift();

if (filesGlob) {
// remove glob from arg list
args = args.filter(function(arg){return arg.indexOf('--glob=') === -1;});

return glob(filesGlob, function(er, files) {
// split file array into chunks of 30
var i,j, chunks = [], chunkSize = 30;
for (i=0,j=files.length; i<j; i+=chunkSize) {
chunks.push( files.slice(i,i+chunkSize));
}

// launch a new process for each chunk
async.series(
chunks.map(function(chunk) {
return function(callback) {
var clangFormatProcess = spawn(nativeBinary,
Copy link
Contributor

Choose a reason for hiding this comment

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

did you confirm that clang-format can't take multiple files in one execution?

Copy link
Contributor

Choose a reason for hiding this comment

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

it does:

If <file>s are given, it reformats the files. If -i is specified
together with <file>s, the files are edited in-place. Otherwise, the
result is written to the standard output.

USAGE: clang-format [options] [<file> ...]

so can we just run the binary a single time? any particular reason you want to do it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I didn't check this at the start and when I did I assumed it would be a bad idea to run the binary with a couple hundred individual files.

That might be a bad assumption though. I'll try running clang-format --glob=node_modules/**/*.js and see if it crashes horribly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think clang-format would have any problem iterating through a large number of files, but you do eventually hit the limit of a command-line argument length.
I think the simple answer is to partition the inputs into batches of ~100 and run clang-format on each batch, without parallelism. Then we don't have to deal with the tough questions like signal handling (ctrl-c kills the children correctly), error spam, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I'll do it like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows seems to have a 260 char path length max, and 8191 char cli command max. Dividing one by the other gives 30, so maybe that's reasonable safe value?

Copy link
Contributor

Choose a reason for hiding this comment

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

The correct fix is to write the paths into a params file, and hand
clang-format a reference to it. But that doesn't seem to be supported.
Your reasoning is good, 30 SGTM

On Fri, Feb 12, 2016 at 6:53 AM Filipe Silva [email protected]
wrote:

In index.js
#23 (comment):

  • // extract glob, if present
  • var filesGlob = args.filter(function(arg){return arg.indexOf('--glob=') !== -1;})
  •                .map(function(arg){return arg.replace('--glob=', '');})
    
  •                .shift();
    
  • if (filesGlob) {
  • // remove glob from arg list
  • args = args.filter(function(arg){return arg.indexOf('--glob=') === -1;});
  • return glob(filesGlob, function(er, files) {
  •  // launch a new process for each file, up to 200 at a time
    
  •  async.parallelLimit(
    
  •    files.map(function(file) {
    
  •      return function(callback) {
    
  •        var clangFormatProcess = spawn(nativeBinary,
    

Windows seems to have a 260 char path length max, and 8191 char cli
command max. Dividing one by the other gives 30, so maybe that's reasonable
safe value?


Reply to this email directly or view it on GitHub
https://github.com/angular/clang-format/pull/23/files#r52749905.

args.concat(chunk),
{stdio: stdio});
clangFormatProcess.on('close', function(exit) {
if (exit !== 0) callback(exit);
else callback(null, exit);
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the first one errors? Or if I give a badly formatted argument to clang-format? does it spew 200 errors to the command line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will at the moment, yeah. They are being run in a parallel up to a limit, and although async will stop spawning more, the output from child processes will still be printed.

I can try looking into not printing that output, or if running it all in one process works then that is fine as well.

});
};
}),
function(err, results) {
if (err) done(err);
console.log('\n');
console.log('ran clang-format with',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the user would rather see
formatted 60 files
instead of
formatted 30 files\nformatted 30 files

Also is there a way to print instead

re-formatting thing1.ts
re-formatting gulpfile.js
re-formatting README.md
re-formatting thing2.js
Checked formatting in 60 files. 4 files reformatted.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the message appearing as duplicate for you? It should only output once, as it does on my setup. The newline is there because otherwise it would appear without separation from the formatted content (when not in -i mode).

As far as I can tell, the only output from clang-format proper is an exit code, so the individual formats (or lack thereof) aren't accessible.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I misread the indent level where the callback is registered, this only happens when async has finished the series?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. If it all of the series elements finish sucessfully, results will have the returns for all the callbacks.

files.length,
files.length === 1 ? 'file path' : 'files path');
done(results.shift() || 0);
});
});
} else {
var clangFormatProcess = spawn(nativeBinary, args, {stdio: stdio});
clangFormatProcess.on('close', function(exit) {
if (exit) done(exit);
});
return clangFormatProcess;
}
}

function main() {
Expand Down
5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@
},
"contributors": [
"Alex Eagle <[email protected]>",
"Martin Probst <[email protected]>"
"Martin Probst <[email protected]>",
"Filipe Silva <[email protected]>"
],
"license": "Apache-2.0",
"dependencies": {
"async": "^1.5.2",
"glob": "^7.0.0",
"resolve": "^1.1.6"
}
}
8 changes: 8 additions & 0 deletions test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ echo "[PASS] absolute path" >&2

FULL_SCRIPT_PATH="$PWD/index.js"
EXPECTED_VERSION_STRING=" at $PWD/testproject/node_modules/" # somewhere in there
EXPECTED_GLOB_STRING="ran clang-format with 1 file path" # somewhere in there

pushd $PWD/testproject
npm install &>/dev/null # Should give us a local clang-format, version doesn't really matter.
Expand All @@ -33,3 +34,10 @@ if [[ $VERSION != *"$EXPECTED_VERSION_STRING"* ]]; then
exit 1
fi
echo "[PASS] file argument anchors resolution" >&2

GLOB=`/usr/bin/env node $FULL_SCRIPT_PATH -i --glob=testproject/lib/**/*.js`
if [[ $GLOB != *"$EXPECTED_GLOB_STRING" ]]; then
echo "[FAIL] Expected string ending in $EXPECTED_GLOB_STRING, got $GLOB" >&2
exit 1
fi
echo "[PASS] glob argument resolution" >&2