Fix almost everything #8

2015-02-08T23:28:47Z

pablomayobre commented

2015-02-08 23:28:47 +00:00

(Migrated from github.com)

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

pablomayobre commented

2015-02-09 02:36:55 +00:00

(Migrated from github.com)

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.

mbrovko commented

2015-02-10 10:32:45 +00:00

(Migrated from github.com)

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

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?
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.
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?

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 commented

2015-02-10 11:25:26 +00:00

(Migrated from github.com)

@insweater

I am also confused my the removal of tostrings.
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.
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

@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](https://github.com/Positive07/gamejoltlua/commit/3fb6f7ce1e4c434ffee7135620e1375c5820240b)

pablomayobre commented

2015-02-10 16:50:48 +00:00

(Migrated from github.com)

Concatenate casts any number to string (didn't thought about booleans though), tables and functions are pointless in this cases.
Yeah, if you have LuaSocket2 then you have http.lua, if you dont have LuaSocket then having http.lua is pointless
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

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. ``` lua 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 ``` lua 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 commented

2015-02-10 17:28:20 +00:00

(Migrated from github.com)

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)

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 commented

2015-02-13 17:45:22 +00:00

(Migrated from github.com)

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

josefnpat commented

2015-02-13 18:07:55 +00:00

(Migrated from github.com)

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.

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 commented

2015-02-14 01:59:43 +00:00

(Migrated from github.com)

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?

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 commented

2015-02-14 11:52:17 +00:00

(Migrated from github.com)

That would be pretty nice.

pablomayobre commented

2015-02-14 17:56:00 +00:00

(Migrated from github.com)

new PR in the way

This repo is archived. You cannot comment on pull requests.

No reviewers