Conversation
|
Thanks for the PR. A few things: The codebase style needs to be honored, so
I really need to setup There are also parts that touch unrelated parts of the codebase; please submit a separate PR for those |
|
I did you the honor.
Any idea how to outsource part of the commits to a separate branch? Also, how did you run the wpt tests? I had to comment out the argument check since deno doesnt allow arguments when testing. |
|
Alright, thanks. Everything mostly looks fine, but I'll check it in more detail later today or tomorrow.
You can create a new branch with git checkout -b new-branch-namerun git reset --hard $COMMIT_HASH_FROM_REFLOGthen the new branch will have the contents of the commit you reverted.
It's in the "Running tests" section of the readme:
|
b-fuze
left a comment
b-fuze
left a comment
There was a problem hiding this comment.
Sorry, I was pretty busy. I noticed a few more things that need to be addressed before I can merge this
| createComment(data?: string): Comment { | ||
| return new Comment(data); | ||
| } | ||
|
|
|
|
||
| // Add parent chain to ancestors | ||
| let parent: Node | null = newParent; | ||
| const parent: Node | null = newParent; |
There was a problem hiding this comment.
Please revert this whole file, it is unrelated to the PR. If you want to submit this, please do it as a separate PR.
| let unitDir = join(dirname(new URL(import.meta.url).pathname), "units"); | ||
| if (Deno.build.os === "windows") { | ||
| unitDir = unitDir.slice(1); | ||
| } |
| const html = await Deno.readTextFile(path); | ||
| const doc = parser.parseFromString(html, "text/html")!; | ||
| let scripts = Array.from(doc.querySelectorAll("script")).map(scriptNode => { | ||
| const scripts = Array.from(doc.querySelectorAll("script")).map(scriptNode => { |
| let count = 0; | ||
|
|
||
| for (const testDir of [wptNodeTests]) { | ||
| for (const testDir of [wptTraversalTests, wptNodeTests]) { |
There was a problem hiding this comment.
This is fine, but WPT tests don't really work atm. We'll need to write some separate basic tests for TreeWalker in test/units
|
|
||
| export interface Filter { | ||
| acceptNode(node: Node): number; | ||
| } |
There was a problem hiding this comment.
Please move this to treewalker.ts
| this.activeFlag = false; | ||
| return result; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Please move this to treewalker.ts
| export class HTMLDocument extends Document { | ||
| constructor() { | ||
| let lock = getLock(); | ||
| const lock = getLock(); |
|
Btw, to revert a full file you can use git checkout 813ce27 ./path/to/file.ts |
|
@b-fuze Any chance you could apply your suggestions to this branch yourself and get this merged? If not, if the staleness of these changes are an issue, I can fork this branch and open a fresh PR. |
|
@chances unfortunately, no. The contributor themselves has to give me access to modify the PR. I'll probably close this and implement it myself at some point. |
|
@b-fuze Access granted |
3 participants
TreeWalker implementation for native,
The wpt/dom/traversal/TreeWalker unittest seems to be broken, it passes all other test.
I hope this helps.