Skip to content
This repository has been archived by the owner on Mar 6, 2019. It is now read-only.

List of improvements #54

Closed
4 of 5 tasks
hhsnopek opened this issue May 17, 2015 · 9 comments
Closed
4 of 5 tasks

List of improvements #54

hhsnopek opened this issue May 17, 2015 · 9 comments

Comments

@hhsnopek
Copy link

Any thoughts on these improvements?

  • Use node's package.json to handle deps and devDeps
  • Use mocha for testing
  • Add travis support for automated testing
  • Use npm scripts for building/testing (npm build & npm test)
  • Update inline documentation to jsdoc (still can use docco for generation, I believe)

These should be looked at prior to merging #47 as the PR contains changes that are unnecessary if any of these improvements are agreed upon.

I'll PR these based on the current master for your review.

@TomNeyland
Copy link

Thats quite a laundry list and rather than hold #47 up waiting on all of that, I would suggest that those changes be made incrementally. Each of those are good changes but they can easily stand on their own as individual pull requests.

Some of the changes in #47 would definitely be unnecessary if all of what you are proposing was already said and done, but given that #47 is ready to go and represents a step in the right direction, I don't necessarily see a convincing reason to wait an undetermined amount of time on the above. And again I think some of those items would be better suited as individual PRs.

@hhsnopek
Copy link
Author

I agree, but if these changes were to be merged in they would undo some of the work that's currently done. I'd rather not create the git history, only to undo it in a week or so.

I see 2 PR's for these changes. The first will complete points one through four. The reasoning is that as I'm migrating the tests over to mocha, pourover.js itself isn't ready for node usage out of the box. That being said I'll be updating pourover.js to be out of the box ready for bower, component, node, and the web. Once the tests and pourover.js are passing we can cute a new release, considering it will do exactly what our original task was of adding bower, component and node support. The second PR I will improve documentation, point five.

All these PRs won't break anything currently using pourover, it will just be a major version bump to 1.1.0 from the first PR and then a minor version bump to 1.1.1. Note: The versioning is based off of semver.

@hhsnopek
Copy link
Author

@TomNeyland I'm going to rebase this onto your fork/branch and send you the PR

@hhsnopek
Copy link
Author

@TomNeyland I have completed points 1, 2, 3 & 4 and have sent you a PR: TomNeyland#4. Unless we'd like these changes to not be apart of your PR.

Notes:

  • All tests are passing, I've changed the release number to 1.1.0 as we added "functionality in a backwards-compatible manner".
  • Publish to Travis will need to be taken care of by the maintainers of NYTimes.
  • Publish to NPM will also need to be taken care of by the maintainers of NYTimes.
  • Point 5 will be done in a separate PR and along with this I will add the Travis Badges.

@tiffehr
Copy link
Member

tiffehr commented Sep 28, 2015

Am I right in that the recommendations listed here are present in some form in @hhsnopek's version? If so, I'll close the Issue with a pointer over to that repo for people who need these additions.

@hhsnopek
Copy link
Author

@tiffehr Yeah, be sure to checkout my community version of pourover which close many of the open issues in this repo. Also I've opened a gitter channel and hold bower/npm ownership atm. It would be awesome if a lot of the upstream changes from my fork could get merged in. Lemme know what you think!

@tiffehr tiffehr modified the milestone: Bower+ support Sep 28, 2015
@hhsnopek
Copy link
Author

Considering that the community edition has these changes, and I'll be mirroring the changes from this repo with other changes. You could close #47

@tiffehr
Copy link
Member

tiffehr commented Sep 29, 2015

Do you want to preserve this history/checklist for your own repo in some fashion?

@hhsnopek
Copy link
Author

I have issues open currently for them, this list was a reference to what was occurring in my fork :)

@tiffehr tiffehr closed this as completed Sep 30, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants