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

[SourceForge] Fixes, tests and cleaning up #1626

Merged
merged 3 commits into from
Apr 2, 2018
Merged

Conversation

PyvesB
Copy link
Member

@PyvesB PyvesB commented Mar 30, 2018

This pull request goes through the following tasks:

  • narrowing down the regex to only match what the implementation actually supports. I believe this should fix Crash in sourceforge #1610.
  • adding some simple tests, which non existent for this service. This should tick one box of the table in The Great Service Rewrite 🏆 #1358.
  • performing a small functional change to remove the /total in the badge display for total downloads. In addition to being slightly odd, it was inconsistent with what other badges do for total downloads.
  • cleaning up the implementation a bit. Nothing too ambitious, but it should help provide a cleaner starting point for Badge request: SourceForge open ticket count #1573.

Looking forward to feedback! 👍

@shields-ci
Copy link

shields-ci commented Mar 30, 2018

Messages
📖

✨ Thanks for your contribution to Shields, @PyvesB!

Generated by 🚫 dangerJS

server.js Outdated
let time_period, start_date;
badgeData.text[0] = getLabel('downloads', data);
// get yesterday since today is incomplete
const end_date = new Date((new Date()).getTime() - 1000*60*60*24);
Copy link
Member

Choose a reason for hiding this comment

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

We have moment installed. Do you know if it might provide more readable ways to express these values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, it does make things more readable! I asserted by adding a temporary log line that the number of downloads returned in the different tests were strictly equal before and after this change.

@@ -5500,37 +5500,35 @@ cache(function(data, match, sendBadge, request) {
}));

// SourceForge integration.
camp.route(/^\/sourceforge\/([^/]+)\/([^/]*)\/?(.*).(svg|png|gif|jpg|json)$/,
camp.route(/^\/sourceforge\/(dt|dm|dw|dd)\/([^/]*)\/?(.*).(svg|png|gif|jpg|json)$/,
Copy link
Member

Choose a reason for hiding this comment

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

👍

@paulmelnikow paulmelnikow added service-badge New or updated service badge sentry Bugs found using Sentry labels Apr 2, 2018
@PyvesB PyvesB merged commit f98d17e into badges:master Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sentry Bugs found using Sentry service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in sourceforge
3 participants