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

npm total downloads support #519

Merged
merged 5 commits into from
Sep 7, 2015
Merged

npm total downloads support #519

merged 5 commits into from
Sep 7, 2015

Conversation

sagiegurari
Copy link
Contributor

This adds support for the total downloads for the given project.
I use the range rest service of npm to fetch the info.

@sagiegurari
Copy link
Contributor Author

any updates on this one?

} else {
badgeData.colorscheme = 'brightgreen';
}
sendBadge(format, badgeData);
Copy link
Member

Choose a reason for hiding this comment

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

Could we include that piece in the try block please?

@espadrine
Copy link
Member

Thanks for contributing! This looks great!

If you could use 2-space indentation to stay consistent with the rest, it would be great.

@sagiegurari
Copy link
Contributor Author

Thanks, I'll be able to do the changes 2 days from now and I'll update the pull request.

@sagiegurari
Copy link
Contributor Author

  1. updated tabbing to 2 spaces
  2. put var index inside for loop definition (although actually not best practice)
  3. expanded the try/catch block to hold all logic
  4. I didn't change the encodeURIComponent to encodeURI since i'm pretty sure its the right thing and its also consistent with rest of the server.js file.

@espadrine
Copy link
Member

I didn't change the encodeURIComponent to encodeURI since i'm pretty sure its the right thing and its also consistent with rest of the server.js file.

In this particular case, it is a bit irrelevant. In the general case, escaping & is not necessary in the path.

See the difference between https://img.shields.io/badge/build-%26passing-brightgreen.svg?label=test%26 and https://img.shields.io/badge/build-&passing-brightgreen.svg?label=test&.

Thanks!

@espadrine espadrine merged commit 6512ec2 into badges:master Sep 7, 2015
@sagiegurari
Copy link
Contributor Author

Thanks for the merge.
I admit that I did this pull request because I want to use that badge :)
When do you deploy changes to your servers?

@espadrine
Copy link
Member

I admit that I did this pull request because I want to use that badge :)

That's the best reason!

When do you deploy changes to your servers?

In 24h. That way, even when I'm on a bad day, I have the next day to realize if there's something I did wrong.

@sagiegurari
Copy link
Contributor Author

Great, thanks a lot.

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