enable traces (sub/child-errors) and custom "error types" #418

oli-obk · 2016-05-08T11:52:03Z

This adds warningMatch and improves functionMatch to allow build providers to emit almost any kind of lint that is specified here: https://github.com/steelbrain/linter/wiki/Linter-API#messages

An example of a modified build-cargo:

fixes #408

noseglid · 2016-05-09T18:14:41Z

lib/error-matcher.js

-        require('xregexp').forEach(this.output, regex, matchFunction.bind(this.currentMatch));
+
+    // first run all functional matches
+    this.functions && this.functions.forEach((f, functionIndex) => {


since this.functions is set in the set function I feel we can safely assume this will always be an array

javascript scares me xD I add checks everywhere. But yea, I'll remove this check

ah, no it's not always set, only if functionMatch is a value.

noseglid · 2016-05-09T18:30:24Z

This amount of code almost warrants moving all linter related code to a linter-integration.js file.

oli-obk · 2016-05-10T10:27:48Z

I removed the criticality, and am parsing the type now. I added a few possible type values. Matches with unknown type are turned into error, Children of matches with unknown type are turned into info.

oli-obk · 2016-05-10T11:11:04Z

This amount of code almost warrants moving all linter related code to a linter-integration.js file.

~~I tried that, I failed (or the linter-integration specs did at least), not sure what I did wrong.~~

oli-obk · 2016-05-10T11:15:23Z

nevermind, I found it :) works now

specs are very weird with their error reporting. Most non-assertion errors just disappear into the void

noseglid · 2016-05-10T11:16:25Z

Shaping up nicely! I'll give this a test and a final review later today!

noseglid · 2016-05-11T19:33:22Z

lib/error-matcher.js

+    }
+    this.regex = {
+      Error: this._prepareRegex(target.errorMatch),
+      Warning: this._prepareRegex(target.warningMatch)


Any reason why the keys here are capitalized?

atom-build currently prints Error capitalized, I'm generating that name from the field name.

Allright, makes sense!

noseglid · 2016-05-11T19:36:29Z

Functionalitywise this is great, but I think we need some more documentation around this, with examples on how to add trace etc. The image you have in this PR would also be nice to spark interest!

oli-obk · 2016-05-11T19:53:16Z

lib/error-matcher.js

+      this.regex[kind] && this.regex[kind].forEach((regex, i) => {
+        regex && require('xregexp').forEach(this.output, regex, (match, matchIndex) => {
+          match.id = 'error-match-' + i + '-' + matchIndex;
+          match.type = kind;


here I use the field name as the type

oli-obk · 2016-05-11T19:56:19Z

I'll think of an introductory example, but I'm not much of a javascript programmer, so my examples might be rather limited.

noseglid · 2016-05-11T20:02:57Z

It doesn't have to be a real life parsing and stuff, just a function match which returns a "match". Should be enough to get started! An entry from the specs might do it

oli-obk · 2016-05-12T11:07:20Z

I added a new example and a spec testing the example

noseglid · 2016-05-12T12:21:54Z

I'm totally happy with the code and docs now.
Something broke on Windows though :(

oli-obk · 2016-05-12T13:10:44Z

~~probably the / path separators, I'll investigate~~

noseglid · 2016-05-12T13:18:04Z

spec/build-error-match-spec.js

+      fs.writeFileSync(directory + 'change_dir_output.txt', fs.readFileSync(__dirname + '/fixture/change_dir_output.txt'));
+      fs.mkdirSync(directory + 'foo');
+      fs.mkdirSync(directory + 'foo/src');
+      fs.writeFileSync(directory + 'foo/src/testmake.c', 'lorem ipsum\naquarium laudanum\nbabaorum petibonum\nthe cake is a lie');


This could be the issue. Should probably use path.join.

oli-obk · 2016-05-12T13:27:28Z

Error Match
  when using function matches
    it should be possible to keep state from previous lines
      Error: ENOTEMPTY: directory not empty, rmdir 'C:\Users\appveyor\AppData\Local\Temp\1\atom-build-spec-116412-2472-1nu667v\foo'
        Error: ENOTEMPTY: directory not empty, rmdir 'C:\Users\appveyor\AppData\Local\Temp\1\atom-build-spec-116412-2472-1nu667v\foo'
        at Error (native)
        at Object.fs.rmdirSync (fs.js:806:18)
        at rmkidsSync (C:\projects\atom-build\node_modules\fs-extra\node_modules\rimraf\rimraf.js:334:11)
        at rmdirSync (C:\projects\atom-build\node_modules\fs-extra\node_modules\rimraf\rimraf.js:324:7)
        at rimrafSync (C:\projects\atom-build\node_modules\fs-extra\node_modules\rimraf\rimraf.js:295:9)
        at C:\projects\atom-build\node_modules\fs-extra\node_modules\rimraf\rimraf.js:332:5
        at Array.forEach (native)
        at rmkidsSync (C:\projects\atom-build\node_modules\fs-extra\node_modules\rimraf\rimraf.js:331:26)
        at rmdirSync (C:\projects\atom-build\node_modules\fs-extra\node_modules\rimraf\rimraf.js:324:7)
        at Function.rimrafSync [as sync] (C:\projects\atom-build\node_modules\fs-extra\node_modules\rimraf\rimraf.js:295:9)
        at Object.removeSync (C:\projects\atom-build\node_modules\fs-extra\lib\remove\index.js:4:17)
        at [object Object].<anonymous> (C:/projects/atom-build/spec/build-error-match-spec.js:58:8)

atom still keeps a handle on a file in the directory while we try to delete the directory after each spec run. This issue is talked about here: isaacs/rimraf#72

Not sure how to fix this, because I have no idea how to close a tab in code. And even then, atom might keep the handle longer than it should.

Can we just stop deleting the specs after running them? They are in a temp directory with a unique id anyway.

noseglid · 2016-05-12T13:40:51Z

But this only happened after your last change.

Oh I just saw you added that test

oli-obk · 2016-05-12T13:50:25Z

I'm actually creating subdirectories. Atom might somehow lock them.

noseglid · 2016-05-12T13:51:24Z

The stack trace is originating from https://github.com/noseglid/atom-build/blob/master/spec/build-error-match-spec.js#L55.
I'd suggest putting a try-catch around that, if we fail to clean up, buhoo. It'll all work anyway and the next tests gets a new temporary directory anyway.

oli-obk · 2016-05-12T13:54:19Z

I'd suggest putting a try-catch around that, if we fail to clean up, buhoo. It'll all work anyway and the next tests gets a new temporary directory anyway.

That's what I meant, for simplicity we can just not clean up at all :) It's not like we're producing huge files during the specs.

noseglid · 2016-05-12T14:02:38Z

I'd feel better with a try-catch than completely removing it though. When I do development I could potentially end up with a lot of temporary files which could've been cleaned up

on windows atom might still have handles to the directory/files if it has a file open in a subdirectory

noseglid · 2016-05-12T18:14:25Z

Just put a comment in the catch

catch (err) {
  // Failed to clean up, ignore this.
}

run npm test locally to lint check it.

oli-obk · 2016-05-12T18:21:35Z

I edited it through the github interface b/c I wasn't on a pc that I had set up ;)

noseglid · 2016-05-12T18:40:48Z

Merged! Thanks so much for this and all your patience!

oli-obk force-pushed the master branch from 23de887 to 0a392f4 Compare May 8, 2016 12:02

oli-obk changed the title ~~enable traces (sub-errors) and custom "error types"~~ enable traces (sub/child-errors) and custom "error types" May 8, 2016

noseglid reviewed May 9, 2016
View reviewed changes

noseglid reviewed May 11, 2016
View reviewed changes

oli-obk reviewed May 11, 2016
View reviewed changes

oli-obk added 15 commits May 12, 2016 12:59

enable traces (sub-errors) and custom error types

a6e7e46

make travis happy

5a0b249

don't focus specs anymore

3d86d2e

more travis nits

d499e35

implement warningMatch

ab1449a

travis

6de354e

enable functionMatch and warningMatch from .atom-build.* files

f5d4c2f

function matches can't be found in the terminal output

d258602

atom-build style is capitalized first letter

a1ec442

errorMatch -> functionMatch breaking change

5a06f52

update docs

726c392

WIP don't report warnings as build-failing

19be0b1

make everything more resilient to accidents in the build script

63401b6

travis

12a856b

remove criticality

20cc3f1

oli-obk added 3 commits May 12, 2016 12:59

split linter code into linter-integration.js

83512de

relative paths are important

028e2df

add a detailed example for functionMatch

f935a0e

oli-obk force-pushed the master branch from 898cbcc to f935a0e Compare May 12, 2016 11:00

noseglid reviewed May 12, 2016
View reviewed changes

oli-obk added 2 commits May 12, 2016 16:25

skip deleting the temporary directory if it fails

a6b94d5

on windows atom might still have handles to the directory/files if it has a file open in a subdirectory

I should go through a javascript tutorial

f0ce2c2

no empty blocks

2651a8f

noseglid merged commit 0c3106e into noseglid:master May 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable traces (sub/child-errors) and custom "error types" #418

enable traces (sub/child-errors) and custom "error types" #418

oli-obk commented May 8, 2016 •

edited

Loading

noseglid May 9, 2016

oli-obk May 10, 2016

oli-obk May 10, 2016

noseglid commented May 9, 2016

oli-obk commented May 10, 2016

oli-obk commented May 10, 2016 •

edited

Loading

oli-obk commented May 10, 2016 •

edited

Loading

noseglid commented May 10, 2016

noseglid May 11, 2016

oli-obk May 11, 2016

noseglid May 11, 2016

noseglid commented May 11, 2016

oli-obk May 11, 2016

oli-obk commented May 11, 2016

noseglid commented May 11, 2016 •

edited

Loading

oli-obk commented May 12, 2016

noseglid commented May 12, 2016

oli-obk commented May 12, 2016 •

edited

Loading

noseglid May 12, 2016 •

edited

Loading

oli-obk commented May 12, 2016 •

edited

Loading

noseglid commented May 12, 2016 •

edited

Loading

oli-obk commented May 12, 2016

noseglid commented May 12, 2016 •

edited

Loading

oli-obk commented May 12, 2016

noseglid commented May 12, 2016

noseglid commented May 12, 2016

oli-obk commented May 12, 2016 •

edited

Loading

noseglid commented May 12, 2016

enable traces (sub/child-errors) and custom "error types" #418

enable traces (sub/child-errors) and custom "error types" #418

Conversation

oli-obk commented May 8, 2016 • edited Loading

noseglid May 9, 2016

Choose a reason for hiding this comment

oli-obk May 10, 2016

Choose a reason for hiding this comment

oli-obk May 10, 2016

Choose a reason for hiding this comment

noseglid commented May 9, 2016

oli-obk commented May 10, 2016

oli-obk commented May 10, 2016 • edited Loading

oli-obk commented May 10, 2016 • edited Loading

noseglid commented May 10, 2016

noseglid May 11, 2016

Choose a reason for hiding this comment

oli-obk May 11, 2016

Choose a reason for hiding this comment

noseglid May 11, 2016

Choose a reason for hiding this comment

noseglid commented May 11, 2016

oli-obk May 11, 2016

Choose a reason for hiding this comment

oli-obk commented May 11, 2016

noseglid commented May 11, 2016 • edited Loading

oli-obk commented May 12, 2016

noseglid commented May 12, 2016

oli-obk commented May 12, 2016 • edited Loading

noseglid May 12, 2016 • edited Loading

Choose a reason for hiding this comment

oli-obk commented May 12, 2016 • edited Loading

noseglid commented May 12, 2016 • edited Loading

oli-obk commented May 12, 2016

noseglid commented May 12, 2016 • edited Loading

oli-obk commented May 12, 2016

noseglid commented May 12, 2016

noseglid commented May 12, 2016

oli-obk commented May 12, 2016 • edited Loading

noseglid commented May 12, 2016

oli-obk commented May 8, 2016 •

edited

Loading

oli-obk commented May 10, 2016 •

edited

Loading

oli-obk commented May 10, 2016 •

edited

Loading

noseglid commented May 11, 2016 •

edited

Loading

oli-obk commented May 12, 2016 •

edited

Loading

noseglid May 12, 2016 •

edited

Loading

oli-obk commented May 12, 2016 •

edited

Loading

noseglid commented May 12, 2016 •

edited

Loading

noseglid commented May 12, 2016 •

edited

Loading

oli-obk commented May 12, 2016 •

edited

Loading