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

try catch finally sendBadge #1356

Closed
wants to merge 2 commits into from

Conversation

RedSparr0w
Copy link
Member

Move the sendBadge(format, badgeData); functions inside of finally blocks,
Don't know if there will be any performance differences, but should minimize the code base slightly.

@paulmelnikow
Copy link
Member

Hey, wanted to say I'm willing to review this if you feel strongly about it.

I've been pushing back for a while on refactoring server.js, since we're about to rewrite all the services as they get broken out into separate modules a la #963. It uses async/await and the handlers return badgeData which basically means the existing logic will be used, but none of the existing structure.

Did you follow that PR? Now that it's merged we ought to open an issue to start discussing how to approach the rewrite. I did make a project but it doesn't have anything useful on it yet.

What are your thoughts?

@paulmelnikow paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Dec 8, 2017
@RedSparr0w
Copy link
Member Author

Ah, completely forgot about that!
No worries, will close this PR.
Will be good to get that going, and a great time to add the currently missing tests too.

@RedSparr0w RedSparr0w closed this Dec 8, 2017
@paulmelnikow
Copy link
Member

Thanks for understanding! And agreed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants