Skip to content
This repository has been archived by the owner on Aug 8, 2020. It is now read-only.

Fix almost everything #8

Closed
wants to merge 11 commits into from
Closed

Fix almost everything #8

wants to merge 11 commits into from

Conversation

pablomayobre
Copy link
Contributor

Fixes issues #1, #2, #4 and #6

It was unnecessary, it comes bundled with LuaSocket
Use BitOps library where available
Added optional args argument to init, args are the arguments passed to
love.load. Used to get the credentials passed by Quick Play
A function to parse the gjapi_credentials.txt file generated by Quick
Play, some other minor changes
More info passed to the developer in order to determine possible errors
Basic cache for trophies
fixed some minor bugs, and fixed setData, now it uses POST requests
A mistake I made in the last commit sorry
@pablomayobre
Copy link
Contributor Author

Fixed #9 too, sorry I made a mistake in the last commit and commited two files that werent part of your project, I have already deleted them. If you want I can start a new Pull request with just one commit, I think that 9 commits is a little too much for the minor changes I made.

Format URL and data values to escape especial character where needed
Now it's the same as the current master branch in kikito's repo
@mbrovko
Copy link
Owner

mbrovko commented Feb 10, 2015

Whoa. You did such a great work.
A few questions I got:

  1. Why did you replace all the tostring(X) by just X? So, you want to pass data into function as strings or what's the point?
  2. I don't agree with removing http.lua. It guess it has to be bundled with library because gamejolt.lua aint' really aimed just for use with LOVE.
  3. The work you've done with LOVE-specific stuff is really nice. I think it won't make problems for those who don't use LOVE, will it?

@josefnpat
Copy link

@insweater

  1. I am also confused my the removal of tostrings.

  2. The http.lua library comes distributed with LuaSocket2, not just LOVE. Marking this library as dependent on the LuaSocket2 library is for the best, unless you intend on using a custom version of http.lua, but you are better off offering pull requests to the maintainer of that library then.

  3. I agree, this library should be able to work with and without LOVE. We could use io.

I think there a lot of commits here, and this whole pull requests should be cherry picked or rebased. Especially because of this commit that was made in error

@pablomayobre
Copy link
Contributor Author

  1. Concatenate casts any number to string (didn't thought about booleans though), tables and functions are pointless in this cases.

  2. Yeah, if you have LuaSocket2 then you have http.lua, if you dont have LuaSocket then having http.lua is pointless

  3. I know that getCredentials is really LÖVE specific, And I can think of a way to make it cross platform.

function GJ.getCredentials(dir)
    local f = io.open(dir.."gjapi-credentials.txt")
    if f then
        GJ.username = f:read()
        GJ.userToken = f:read()
        return true
    else
        return false
    end
end

You would call it like this in LÖVE

GJ.getCredentials(love.filesystem.getWorkingDirectory().."/")

I know that there are too many commits, I want to make all the changes, delete my repo and create it again, then submit all the changes in one commit, I want to know if you consider any of the changes is not good like you did here

@pablomayobre
Copy link
Contributor Author

Okey I checked, there are two cases where I really deleted the tostring function, in GJ.fetchTrophy, the id parameter, but id must be a number or else there would be an error so... I'm not sure

And the limit parameter in GJ.fetchScore which I'm not sure at all but I guess should be a number.
Maybe that part should be changed to (tonumber(limit) or 10)

@mbrovko
Copy link
Owner

mbrovko commented Feb 13, 2015

If you to remove http.lua from repo, shall we also remove md5.lua?

@josefnpat
Copy link

So long as the dependency is easily identified and linked, I see no problem in removing http.lua and md5.lua so long as this project maintains itself against the most recent version of md5.luq.

@pablomayobre
Copy link
Contributor Author

Yeah, and in installation you write that md5.lua file should be in the same folder as gamejolt.lua, and put a link to the md5.lua repo, or a way to install it through luarocks (which is also totally possible now).

Should I create a new PR with all this changes?

@mbrovko
Copy link
Owner

mbrovko commented Feb 14, 2015

That would be pretty nice.

@pablomayobre
Copy link
Contributor Author

new PR in the way

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants