Skip to content
This repository was archived by the owner on Jul 31, 2018. It is now read-only.

Formatting for 002 ES Modules#23

Closed
bmeck wants to merge 5 commits intonodejs:masterfrom
bmeck:es6-module
Closed

Formatting for 002 ES Modules#23
bmeck wants to merge 5 commits intonodejs:masterfrom
bmeck:es6-module

Conversation

@bmeck
Copy link
Member

@bmeck bmeck commented May 18, 2016

  • numbers for headings
  • rewording algorithms to mimic TC39 style for VM implementors
  • removal of unnecessary fluff
  • adds back path searching after WHATWG/TC39 meeting

* **[HostResolveImportedModule]**
- A hook for when an import is exactly performed.
* **[HostResolveImportedModule](https://tc39.github.io/ecma262/#sec-hostresolveimportedmodule)**
- A hook for when an import is exactly performed. This returns a `ModuleRecord`. Used as a means to grab modules from node's loader/cache.
Copy link
Member

Choose a reason for hiding this comment

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

s/node/Node

@rvagg
Copy link
Member

rvagg commented May 19, 2016

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
Copy link
Member Author

bmeck commented May 19, 2016

@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
Copy link
Member Author

bmeck commented May 19, 2016

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

@rvagg
Copy link
Member

rvagg commented May 19, 2016

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

7. Let `value` be ! `O`.`[[Get]]`(`P`,` O`)
8. Return `value`

#### 3.2.2. `DelegatedModuleNamespaceObjectCreate(module,O)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a space after the comma?

@trevnorris
Copy link
Contributor

@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
Copy link
Member

rvagg commented May 20, 2016

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

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

@bmeck
Copy link
Member Author

bmeck commented May 20, 2016

@trevnorris fixed

@trevnorris
Copy link
Contributor

LGTM

trevnorris pushed a commit that referenced this pull request May 20, 2016
Change the document's formatting, numbering and style to match the TC39
spec algorithm explanation.

Clarify [[Environment]] and [[Realm]]

PR-URL: #23
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@trevnorris
Copy link
Contributor

Thanks much! Squashed and landed in 2272a81.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants