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

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

@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

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

@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

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

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

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

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

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

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

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

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

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 Label
Milestone
No items
No Milestone 0.2.6
Projects
Clear projects
No project
Assignees
Clear assignees
iLoveElysia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#145
Delete Branch "fix/bugs-and-add-tests"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?