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

Removed the need to hardcode Ludum Dares #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ajayyy
Copy link
Contributor

@ajayyy ajayyy commented Jan 2, 2019

This was fun to do, I have not used promises before, so it took a lot of learning.

This assumes the "event-finished" data point in the api means that the ratings are submitted. I will double check this. This deprecates #26

If you want to wait to be certain that is the correct data point, you can just merge #26 for now.

@ajayyy ajayyy mentioned this pull request Jan 2, 2019
@pjnovas
Copy link
Owner

pjnovas commented Jan 2, 2019

@ajayyy nice feature!, the only thing I would like to get there is unit-tests, I've been coding that module using TDD and I don't wanna loose that.

And lastly it would be cool to have the LD number being discovered and cached. One of the "why" I set up that "hardcore" in there is because LD data is fetched only once (when LD judge ends) and it will never change later on, so it may be too much to be fetching all LDs on every user fetch.
But I'll check this out later

@ajayyy
Copy link
Contributor Author

ajayyy commented Jan 2, 2019

Now that I know the info from #29, this is not finished as this does not pull that the totals from the api.

How do I do the unit tests?

@pjnovas
Copy link
Owner

pjnovas commented Jan 2, 2019

oh kk, no problem.

So for testing, you should be able to run all test by npm test and you can find all the related tests here: https://github.com/pjnovas/ldstats/tree/master/test.

If you have never right tests, it may take you some time, I can work on them tho but I'm not sure when I would have time for it

@ajayyy
Copy link
Contributor Author

ajayyy commented Jan 3, 2019

Thanks!

So, would you rather this not be automatic, as it would not be cached?

Edit: Ooh, I just noticed that you use my game in the unit tests :)

@pjnovas
Copy link
Owner

pjnovas commented Jan 5, 2019

@ajayyy hey so yeah it would make more sense to cache that instead of fetch every time.

I have added the latest LD into the api and deployed with your pull request #27. I have refactored the code a little (which is the file with conflicts here) and it looks like the only needed thing to store on ldstats is the eventId of the LD then totals could be cached by calling the api with that stored eventId and that would be removing the config.json with all the totals LD.

I cannot find the time to work on that right now but I will definitely review, merge and publish if you wanna take it.

Thanks again for the work you put there ;)

@ajayyy
Copy link
Contributor Author

ajayyy commented Jan 7, 2019

Awesome, thanks for the work you've done on ldstats, it's really an awesome site. I will try to do that in a few days if I get some time, but school is starting back up for me and I'll be off my break, so I might not.

There is still all the way until April now until this matters again. :)

@DDRKirbyISQ DDRKirbyISQ mentioned this pull request Oct 28, 2020
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