search

adobe / brackets-edge-web-fonts

Edge Web Fonts extension for Brackets. Simply unzip and drop into your Brackets extension folder to browse and include Edge Web Fonts.
http://html.adobe.com/edge/webfonts/
MIT License
50 stars 20 forks source link

Add app name to include generation if running in Edge Code #52

Closed joelrbrandt closed 11 years ago

gruehle commented 11 years ago

Done with initial review. Just one minor comment.

joelrbrandt commented 11 years ago

@gruehle Thanks for the quick review! I removed the console.log.

I also added this pedantic thought, which is hidden because I deleted the line in question:

I'm all for keeping the console clean, so I'll take this line out.

However, because I love being pedantic: I thought about this when I wrote it. If the user hits the "catch" case, we are catching an actual bug that should be fixed. It seems to me that it's good programming practice to make sure that actual bugs have an outwardly-visible symptom. (Though it's not clear that this console.log is the right symptom.)

gruehle commented 11 years ago

I guess I would argue that an exception here is always indicative of a bug. The required functionality is having the appname script included when run from Edge Code (or any other brackets derivative whose name ends with " code"). That result is outwardly visible--you either have the appname script or you don't.

Merging.