nodejs / node-eps

Node.js Enhancement Proposals for discussion on future API additions/changes to Node core
442 stars 66 forks source link

Formatting for 002 ES Modules #23

Closed bmeck closed 8 years ago

bmeck commented 8 years ago
rvagg commented 8 years ago

lgtm sans inline nits, I also don't like the heavy inconsistency in line lengths (personally I'd prefer paragraphs on a single line but it's consistency I care about more) but not enough to hold this up, just a note for future process improvement perhaps.

bmeck commented 8 years ago

@rvagg https://tc39.github.io/ecma262/ doesn't always highlight names, was trying to match them, though saw no direct style guide, will amend though

bmeck commented 8 years ago

@rvagg fixed, though some lines still exceed 80, (code and href)

rvagg commented 8 years ago

You can argue line-length with @trevnorris and @bnoordhuis, they are the ones who use XGA CRT monitors which have limitations, I'm easy as long as it looks consistent.

lgtm, happy to see this landed after the usual delay, would appreciate additional review of course.

xga

trevnorris commented 8 years ago

@bmeck Minor comment that I can easily fix before merging.

@rvagg My line limit arguments only apply to code. Just find it convenient for review purposes with text. :)

rvagg commented 8 years ago

It's OK @trevnorris, you don't need to explain

ibm-8512-crt-vga-mcga-ps2-personal-system2-monitor-display_121892247461

bmeck commented 8 years ago

@trevnorris fixed

trevnorris commented 8 years ago

LGTM

trevnorris commented 8 years ago

Thanks much! Squashed and landed in 2272a8138a.