Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A bunch of minor fixes I grouped together #19

Merged
merged 11 commits into from
Mar 9, 2017

Conversation

danii-nebot
Copy link
Contributor

  • sys is deprecated: allow 'sys' to be an alias for 'util' nodejs/node-v0.x-archive#3577, use utils instead

  • Add npm log file to .gitignore as it just clutters the repo

  • Fix typo in searchBooks example

  • Add build npm script

  • I believe it's best practice to remove JavaScript files from the repo and ignore them, since they are just compiled from the CoffeeScript source

Copy link
Collaborator

@rharriso rharriso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdickason @danii-nebot Most of these changes look pretty good. With a couple of suggestions.

.gitignore Outdated
# compiled JS
*.js
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we target the build folder instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing, I agree that should be safer :)

README.md Outdated
@@ -12,17 +12,18 @@ Installation
======
1. Install npm: `curl http://npmjs.org/install.sh | sh`
2. Grab this module from npm: `npm install goodreads`
3. Include it in your program:
3. Compile Coffeescript into JavaScript files using `npm run build`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not happen after installation?

Copy link
Contributor Author

@danii-nebot danii-nebot Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad I mixed up installation instructions from npm with running the lib after cloning from github. Yeah this is not necessary, specially if we set up the prepublish hook discussed later.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't want to include in this PR. just open an issue and link to it in the thread.

console.log 'searching for book' + q
gr.searchBooksq, (json) ->
console.log 'searching for book' + q
gr.searchBooks q (json) ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah, thanks

@@ -3,6 +3,9 @@
"description": "Wrapper for the Goodreads API",
"version": "0.1.0",
"author": "Brad Dickason <dickason@gmail.com> (http://braddickason.com)",
"scripts": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this needs to go into a prepublish hook.

@danii-nebot
Copy link
Contributor Author

Thanks for your suggestions, I tried to take care of everything.
Please note that the Cakefile is currently being published in the npm package. I wasn't sure if I should add it to .npmignore as well

@rharriso rharriso merged commit c8a8805 into bdickason:master Mar 9, 2017
@danii-nebot danii-nebot deleted the style-points branch March 10, 2017 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants