iLoveElysia/pjax
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 9 Wiki Activity

Fix bugs and add tests #145

Merged
BehindTheMath merged 11 commits from fix/bugs-and-add-tests into master 2018-04-09 22:36:33 -05:00
Conversation 12 Commits 11 Files Changed 22
+478 -153
BehindTheMath commented 2018-03-23 10:24:08 -05:00 (Migrated from github.com)
Copy link
Copy source

This PR has a bunch of various bug fixes for the code and tests, and many new tests. It's based on #144, so that needs to be merged first.

@robinnorth I cherry-picked your commit from the fix/missing-tests branch (Add tests for lib/util/noop.js - 5e7341d).

This PR has a bunch of various bug fixes for the code and tests, and many new tests. It's based on #144, so that needs to be merged first. @robinnorth I cherry-picked your commit from the `fix/missing-tests` branch (Add tests for `lib/util/noop.js` - 5e7341d).
robinnorth commented 2018-03-27 08:41:50 -05:00 (Migrated from github.com)
Copy link
Copy source

@BehindTheMath nice one, that's a substantial PR! I'll take a proper look at it as soon as I have time.

@BehindTheMath nice one, that's a substantial PR! I'll take a proper look at it as soon as I have time.
BehindTheMath commented 2018-03-27 08:45:32 -05:00 (Migrated from github.com)
Copy link
Copy source

Thanks. I didn't originally plan on it being so big, but I was aiming for as close to 100% coverage as I could, and I kept adding more tests and finding more issues.

If it's too big, I can break it up into smaller PRs.

Thanks. I didn't originally plan on it being so big, but I was aiming for as close to 100% coverage as I could, and I kept adding more tests and finding more issues. If it's too big, I can break it up into smaller PRs.
BehindTheMath commented 2018-04-09 09:42:00 -05:00 (Migrated from github.com)
Copy link
Copy source

@robinnorth Do you think you'll have time to look this over, or should I just merge it? I would love to have a second pair of eyes on it, but I know you said you've been busy lately.

@robinnorth Do you think you'll have time to look this over, or should I just merge it? I would love to have a second pair of eyes on it, but I know you said you've been busy lately.
robinnorth commented 2018-04-09 09:52:03 -05:00 (Migrated from github.com)
Copy link
Copy source

Sorry, I've been very tardy in reviewing this for you. I have indeed been busy, but I'll make some time to check it out later and get back to you.

Sorry, I've been very tardy in reviewing this for you. I have indeed been busy, but I'll make some time to check it out later and get back to you.
BehindTheMath commented 2018-04-09 09:56:21 -05:00 (Migrated from github.com)
Copy link
Copy source

Thank you, I appreciate it.

Thank you, I appreciate it.
robinnorth (Migrated from github.com) reviewed 2018-04-09 15:06:00 -05:00
lib/proto/parse-element.js
@@ -1,15 +1,17 @@
var attrState = "data-pjax-state"
robinnorth (Migrated from github.com) commented 2018-04-09 15:06:00 -05:00
Copy link
Copy source

This, and line 12 could be abstracted to use var attrState = "data-pjax-state", as in attach-link

This, and line 12 could be abstracted to use `var attrState = "data-pjax-state"`, as in `attach-link`
robinnorth (Migrated from github.com) reviewed 2018-04-09 15:17:22 -05:00
tests/lib/proto/attach-form.js
robinnorth (Migrated from github.com) commented 2018-04-09 15:17:22 -05:00
Copy link
Copy source

Is attachFormk a typo?

Is `attachFormk` a typo?
BehindTheMath (Migrated from github.com) reviewed 2018-04-09 15:18:05 -05:00
tests/lib/proto/attach-form.js
BehindTheMath (Migrated from github.com) commented 2018-04-09 15:18:05 -05:00
Copy link
Copy source

Yup

Yup
BehindTheMath (Migrated from github.com) reviewed 2018-04-09 15:18:15 -05:00
lib/proto/parse-element.js
@@ -1,15 +1,17 @@
var attrState = "data-pjax-state"
BehindTheMath (Migrated from github.com) commented 2018-04-09 15:18:15 -05:00
Copy link
Copy source

True.

True.
robinnorth (Migrated from github.com) approved these changes 2018-04-09 15:41:20 -05:00
robinnorth (Migrated from github.com) left a comment
Copy link
Copy source

I've finished stepping through all the commits, run the example project and tested against my real-world project that's using this and everything seems good to merge to me!

I've finished stepping through all the commits, run the example project and tested against my real-world project that's using this and everything seems good to merge to me!
BehindTheMath commented 2018-04-09 22:38:53 -05:00 (Migrated from github.com)
Copy link
Copy source

I deleted the fix/missing-tests branch, since I cherry-picked the only commit from there into this PR, and we're close to 100% coverage.

I deleted the `fix/missing-tests` branch, since I cherry-picked the only commit from there into this PR, and we're close to 100% coverage.
robinnorth commented 2018-04-10 03:04:02 -05:00 (Migrated from github.com)
Copy link
Copy source

Cool, thanks!

Cool, thanks!
Sign in to join this conversation.
Reviewers
No Reviewers
BehindTheMath
robinnorth
Labels
Clear labels
bug
dependencies
Pull requests that update a dependency file
 duplicate
enhancement
invalid
question
wontfix
No labels
Milestone
No items
No Milestone 0.2.6
Projects
Clear projects
No projects
Assignees
Clear assignees
iLoveElysia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#145