angular / batarang

AngularJS WebInspector Extension for Chrome
MIT License
2.43k stars 338 forks source link

Appropriately process messages received via postMessage #222

Closed jvilk closed 9 years ago

jvilk commented 9 years ago

Chrome extensions are fun, aren't they? :)

Hope this helps you out. :)

Review on Reviewable

jvilk commented 9 years ago

Oops, looks like I forgot to fix the mock objects, leading to Travis failures! I can fix that in a couple of hours.

SomeKittens commented 9 years ago

Thanks for the PR (and apologies for not getting to this sooner). Can you rebase this into one commit following our commit rules?

jvilk commented 9 years ago

Sure. I'll do that tonight.

SomeKittens commented 9 years ago

Awesome, thanks again!

jvilk commented 9 years ago

OK, I've squashed and changed the commit to follow your commit rules. @SomeKittens, you should link to the official AngularJS contributing guide in the Batarang's CONTRIBUTING.md! Had it been linked / mentioned before, I would have opened the PR in the appropriate format from the start. :-)

If you want, I can open up a PR to do that for you, but it looks like that doc could use some loving. Looks like @btford was a bit lazy when he put it together; maybe we should give him more work to do. :wink:

jvilk commented 9 years ago

Also, the build process for the Batarang appears to update files in the dist folder. I did not include these in the PR, despite them being under version control. Perhaps you should mention what to do about those in the contributing guide, as well.

SomeKittens commented 9 years ago

Hmm, didn't realize our CONTRIBUTING was out of date. Wouldn't it be nice if you could symlink across GitHub?

Not including the dist files is fine. Us contributors are still on the fence about it. (and as you say, it'll all be overridden by builds anyway.

I'm halfway through debugging https://github.com/angular/angular-hint/issues/88 so my index is dirty. Once I clear that up I'll be able to bring this in. Pleasure doing business with you.

SomeKittens commented 9 years ago

Hello again! Serious apologies for not getting to this sooner.

We do emit events that don't have the severity property (see https://github.com/angular/angular-hint/blob/master/src/modules/scopes.js#L42 ), so this will result in false negatives. (That's our documentation fail - working on it...)

jvilk commented 9 years ago

@SomeKittens ah OK, I thought I grepped for all emit calls.

Which fix would you prefer?

SomeKittens commented 9 years ago

I'd prefer __fromBatarang as that's very clear to anyone reading it what it means.

jvilk commented 9 years ago

I looked into this, @SomeKittens, and it looks like there's no problem (correct me if I'm misunderstanding the source code):

Thus, I believe every postMessage event has a severity property.

I did modify the commit to add the explicit __fromBatarang property, though, as I believe that is a clearer way of identifying your messages.

SomeKittens commented 9 years ago

It'll be there - it just might be falsy.

consensus is __fromBatarang, will try and get to this tonight.

jvilk commented 9 years ago

poke

SomeKittens commented 9 years ago

Landed in 56c3ad7162c9edef8f6cddb578a48c80977faaed