Code cleanup #120
Assignees
iLoveElysia
Clear assignees
iLoveElysia
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: iLoveElysia/pjax#120
Reference in New Issue
Block a user
Delete Branch "cleanup/misc-cleanup"
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?
All the proposed changes look good to me!
There are a few more long-standing code comments that could possibly do with removing as part of a cleanup too -- things like commented-out module require calls and disabled experimental code. https://github.com/MoOx/pjax/blob/master/lib/switches.js has a lot of examples of this. https://github.com/MoOx/pjax/blob/master/lib/switches-selectors.js#L15-L20 and https://github.com/MoOx/pjax/blob/master/index.js#L12 also spring to mind.
Finally, do we still need the polyfill for
Function.prototype.bind? AFAIK it's supported on all platforms that also fully support the History API.I'll take a look at those later.
BTW, you should also be able to push commits to this PR. Just checkout upstream/cleanup/misc-cleanup, and then push your commits back to the upstream branch.
Sure, thanks! I made comments rather than commits to avoid stepping on your toes, but if you like, I can action the changes I suggested. Equally happy to leave them to you, if you like.
Everything looks good, but I think we should leave this open until we're ready to release 0.2.5, in case there's anything we want to add.
Sounds good to me, hopefully we can get 0.2.5 out fairly soon!
I've actioned the items I suggested previously and rebased against master to keep this PR up-to-date. I can't think of anything else that needs cleaning up at present, other than possibly the description in
package.json(and adding acontributorsattribute?).@@ -13,12 +13,12 @@ module.exports = function(el) {script.type = "text/javascript"@robinnorth I'm confused. Why did you change this back? I thought we agreed it was synchronous.
(I think it's probably time for 1.0 ;) )
@@ -13,12 +13,12 @@ module.exports = function(el) {script.type = "text/javascript"Oops, I guess I didn't quite successfully resolve that merge conflict... Will fix.
@robinnorth What do you think about that last commit (
2ec4c14)?It looks like after this PR is merged, all issues milestoned for 0.2.5 will be closed.
Is there anything else that needs to be done before working on releasing 0.2.5?
The library was obviously modularised with testability in mind, but these modules have never had corresponding tests, so it's probably OK to inline them like this.
Only a version bump and any changes to
package.json(I added you and I as contributors, hope that's OK) and then we're good to go! Do you have npm publish access?@MoOx Should we add everyone who contributed (https://github.com/MoOx/pjax/graphs/contributors)?
I do not. I'll update the changelog and version number, and then @MoOx can publish it.
With that in mind, should we move out
loadUrl(),loadContent(), andafterAllSwitches()to separate files?It would be great to modularise and be able to independently test these methods, as they're basically the core of the whole library, but it's probably going to be a bit of an undertaking to do so right now. I think we should get this next release out and then add this as an aim for the next major milestone (and close #21 in the process).
Sounds good.
So we're just waiting on @MoOx for an answer about other contributors, and then we can merge this. I have a branch with the updated changelog and the version bump that I'll rebase and push once this is merged.
Do whatever you feel appropriate.
I can give you npm access if you want or I can release it too. Just tell me what you prefer.
To make changelog easier I would recommend you in the future to only use "Squash and merge" option when merging PR. That helps to have a clean git history with kind of "one pr = one feature or a bugfix" history, which is easy to review and create release notes from.
If you can give me access, we won't have to bother you in the future. I'll DM you on Twitter.
I wasn't sure which to use, since it looks like in the past sometimes merge commits were used instead of squashing. Also, using merge commits doesn't require as much rebasing. But we can start squashing.
You have been added to npm ;)
For the merging, it's a recommendation. At the time I made this lib, not sure it was a thing :D
Great, thanks.
@robinnorth What do you think? Should we add anyone else?
@BehindTheMath it's up to you -- we could add/ask to add anyone who has ever made a contribution via a PR and add a note encouraging future contributors to add themselves to the list when making a PR, or we could just list contributors who have made a significant contribution (i.e. multiple PRs)/have commit access to the repo, or leave it as is. My personal inclination is that one PR doesn't really qualify for 'contributor' status, but you were here before me, so I'll defer to you on this 😄
I don't want to slight anyone by leaving them out, but at the same time I think it will get unwieldy if there's a huge list of everyone who submitted commits.
I'm going to leave it as it is, and if anyone wants, they can add themselves in.
I think that's a sensible approach 👍