-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
programs/github
Outdated
| local action = select(1, ...) | ||
|
|
||
| local usage = "Usage: github clone <user>/<repo> [<branch>|<tag>] [<destination>]" | ||
| local usage = "Usage: github clone <user>/<repo> [-b <branchname>] [-t <tagname>] [-a <username>:<password>] [<destination>]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[-b <branchname> | -t <tagname>]
|
It's tempting just to throw |
|
hold a tick... push incoming |
|
should be good now. |
programs/github
Outdated
| if arg == flag then | ||
| isFlag = true | ||
| flagName = flagType.name | ||
| return isFlag, flagName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just return flagType.name
|
Scratch that, |
| end | ||
|
|
||
| -- get flag arguments | ||
| local function findFlag(arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's an improvement, but this whole function could be written:
local flagNames = {
['--branch'] = 'branch',
['-b'] = 'branch'
['--tag'] = 'tag',
['-t'] = 'tag',
['--auth'] = 'auth'
['-a'] = 'auth'
}
local findFlag = function(arg) return flagNames[arg] end|
ready for CR again |
|
I am now accounting for your purposed I also REALLY liked your simplification of the |
programs/github
Outdated
| end | ||
|
|
||
| if flags.branch and flags.tag then | ||
| return printError('please specify a branch or tag.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eric-wieser I'm not sure how I feel about the error message here... care to offer a better one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this conveys the wrong message. "--branch and --tag cannot both be specified" would be more accurate.
Note that git clone doesn't actually "support" tags, but git clone -b a_tag tends to do the right thing anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah and honestly providing the tag argument is more of a formality anyway.
Either branch or tag are going to be used for treeName so it's really just
calling the same variable a different name.
On Thu, Aug 25, 2016, 17:42 Eric Wieser notifications@github.com wrote:
In programs/github
#11 (comment)
:+local passed = {...}
+local args = {}
+local flags = {}
+local flagValue = ''
+for k, arg in pairs(passed) do
- local flagName = findFlag(arg)
- if flagName and arg ~= flagValue then
flags[flagName] = passed[k + 1]flagValue = flags[flagName]- elseif arg ~= action and arg ~= flagValue then
table.insert(args, arg)- end
+end
+if flags.branch and flags.tag then
- return printError('please specify a branch or tag.')
Yeah, this conveys the wrong message. "--branch and --tag cannot both be
specified" would be more accurate.Note that git clone doesn't actually "support" tags, but git clone -b
a_tag tends to do the right thing anyway—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/eric-wieser/computercraft-github/pull/11/files/0a9cc032fea14dc4adacf2e0d7a51516dbda88e9#r76338340,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA41FnctuDfo_3j-arTEwoTrNKlhxfHfks5qjho4gaJpZM4JsvBA
.
|
ready for another CR. So now I am accounting for the |
|
I've rebased most of these into a few commits, which I've pushed to I'd encourage you to |
|
done. On Thu, Aug 25, 2016 at 10:55 PM Eric Wieser notifications@github.com
|
|
tl;dr: don't merge this yet it doesn't work. bad news on this one... the auth part of this completely doesn't work... I tested it in the browser as well and apparently user:pass@api.github.com doesn't count as an authenticated request :( I think I have a way to solve this but it'll have to wait until tomorrow. In short... don't merge this PR yet. |
|
@eric-wieser actually in the interest of resolving #8 I've removed all the auth logic for now... at least this branch provides the support structure I'll need to build that out. So go ahead and CR and merge this one as a simple resolution to #8 and I'll put in a new PR tomorrow or this weekend for the auth stuff. I've already squashed this PR down for you :) |
|
have you had a chance to CR this @eric-wieser or should I just build the support-auth with a dependency on this? Its been tested and it works. |
|
bump |
|
You can build with a dependency on this anyway - I can always rebase that PR after merging this one |
|
okie dookie... this is already squashed, and tested... literally just needs a CR and merge... |
| local function findFlag(arg) | ||
| if arg.sub(1,1) == '-' then | ||
| if not acceptedFlags[arg] then | ||
| return printError(('Unknown flag argument %s'):format(arg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should print the usage too. Also, does this actually exit, or keep running anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're returning a value it should exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but it only exits this function. The script will continue and run, won't it, treating this as the dest/repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me look into the printError() function I assumed it would terminate
the program.
On Sat, Aug 27, 2016, 21:00 Eric Wieser notifications@github.com wrote:
In programs/github
#11 (comment)
:
- if not dest then
dest = treeName +local function findFlag(arg)treeName = 'master'- if arg.sub(1,1) == '-' then
if not acceptedFlags[arg] thenreturn printError(('Unknown flag argument %s'):format(arg))Right, but it only exits this function. The script will continue and run,
won't it, treating this as the dest/repo?—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/eric-wieser/computercraft-github/pull/11/files/78509095ce2a421a6e2f7dd6d245dcc050539025#r76524809,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA41FoXaVaR7Wmk1f6zugjhbdAUseYZZks5qkOvOgaJpZM4JsvBA
.
|
@eric-wieser sorry for taking so long RL caught up to me... It still needs to go through some testing but I'd love to get some feed back on e24c416 |
|
ok @eric-wieser first round of fixes is up give it a look, lets try to keep the conversation to architecture until I can run tests on it tonight :) |
apis/github
Outdated
| f.write(textutils.serialize(authTable)) | ||
| f.close() | ||
| end | ||
| Auth.delete = function (user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn it why is there a space here :(
|
I updated the PR comments to reflect the fact that we are indeed adding support for authentication in this PR after all :D |
readme.md
Outdated
| apis programs | ||
| github readme.md | ||
|
|
||
| #### Adding Authentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eric-wieser feel free to give me feedback on the verbiage here... Have you played with GH's new code review stuff? Its seems kind of cool. You can start a review instead of "Add single comment" and it will wait for you to finish commenting and submit all your comments as one message.
aaronmallen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conversation starter and small changes needed. @eric-wieser
programs/github
Outdated
|
|
||
| if action == 'auth' then | ||
| local user, token = args[1], args[2] | ||
| if not user or not token then return printError("No user or token specified.") end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also verify the delete argument wasn't specified, so a user can simply do auth -D aaronmallen and the authentication data will be deleted without having to specify user and token.
programs/github
Outdated
| print(('Github authentication for %s successfully deleted.').format(flags.delete)) | ||
| else | ||
| auth.save() | ||
| print('Github user saved.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eric-wieser should we include what user was saved in this comment?
programs/github
Outdated
|
|
||
| if not action == 'clone' or not action == 'auth' then | ||
| print('Clone usage: %s').format(usage['clone'])) | ||
| print('Auth usage: %s').format(usage['auth'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why am I having such a hard time with the tabs on this freaking project?
| local authTable = Auth.getAll() | ||
| return authTable[user] | ||
| end | ||
| Auth.save = function(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to verify it is reasonable to require Auth.new in order to invoke auth.save() right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's pretty fair
| f.write(textutils.serialize(authTable)) | ||
| f.close() | ||
| end | ||
| Auth.delete = function(user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function assumes a user was deleted even if the user doesn't exist... this shouldn't be an issue but I'm not sure how you feel about that.
programs/github
Outdated
|
|
||
| if not validActions[action] then | ||
| for k, v in pairs(validActions) do | ||
| print(('%s usage: %s').format(v.name, v.usage)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terminal width is a premium here, so I'm -1 on printing anything other than the usage string
readme.md
Outdated
| #### Adding Authentication | ||
| To use authenticated requests you must first [create a github](https://help.github.com/articles/creating-an-access-token-for-command-line-use/) api token on your github account. | ||
|
|
||
| github auth <user> <token> [-D <username>] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be github auth <user> [<token>| -d] in my opinion. The implementation too, obviously. Printing out the token with just the user argument would be handy too
readme.md
Outdated
| github readme.md | ||
|
|
||
| #### Adding Authentication | ||
| To use authenticated requests you must first [create a github](https://help.github.com/articles/creating-an-access-token-for-command-line-use/) api token on your github account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to recommend a list of scopes here, to avoid damage (once you work out which ones are needed)
readme.md
Outdated
|
|
||
| The delete argument is optional and will delete specified user when defined. The user and token arguments maybe undefined when using the delete option. | ||
|
|
||
| **Warning:** data provided to `github auth` will be stored locally on the computercraft computer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth mentioning how to revoke tokens as part of this warning.
programs/github
Outdated
| if not user or not token then return printError("No user or token specified.") end | ||
| local auth = github.Auth('basic', user, token) | ||
| auth.save() | ||
| print(('Github user %s successfully saved.').format(auth.user)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Successfully saved github token for user %s" is a better message IMO
programs/github
Outdated
| -- parse input | ||
| local repo, treeName, dest = select(2, ...) | ||
| -- get flag arguments | ||
| local acceptedCloneFlags = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this be folded into validActions?
programs/github
Outdated
| ['--delete'] = 'delete' | ||
| } | ||
|
|
||
| local function getFlagTable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which would make this just be actions[action].flags
| ['--tag'] = 'tag', | ||
| ['-a'] = 'authed', | ||
| ['-auth'] = 'authed' | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screwy indentation strikes again! (two spaces for clone, yet tabs here)
programs/github
Outdated
| @@ -134,24 +128,26 @@ if action == 'clone' then | |||
| end | |||
|
|
|||
| if action == 'auth' then | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: add action = validActions[action] up where actions is defined, then compare against validActions.auth. Then nothing else needs to know about the validActions table, and getFlagTable() becomes pointless
readme.md
Outdated
| To use authenticated requests you must first [create a github](https://help.github.com/articles/creating-an-access-token-for-command-line-use/) api token on your github account. You do not need to provide any api scopes for the token unless you plan on accessing private repositories. | ||
|
|
||
| github auth <user> <token> [-D <username>] | ||
| github auth <user> [<api token> | -D] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue lowercase d because 1) it's easier to type, 2), git branch -d <name> is a thing
readme.md
Outdated
| The delete argument is optional and will delete specified user when defined. The user and token arguments maybe undefined when using the delete option. | ||
|
|
||
| **Warning:** data provided to `github auth` will be stored locally on the computercraft computer. | ||
| **Warning:** data provided to `github auth` will be stored locally on the computercraft computer. You can delete the access token at anytime by hitting the delete button in your personal access tokens screen on github. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link to that page? https://github.com/settings/tokens, right?
programs/github
Outdated
| github.Auth.delete(flags.delete) | ||
| print(('Github authentication for %s successfully deleted.').format(flags.delete)) | ||
| github.Auth.delete(user) | ||
| print(('Successfully deleted github token for user %s'):format(flags.delete)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we can probably cut the world "Successfully` (here and below), given our terminals are narrow
programs/github
Outdated
| @@ -1,15 +1,28 @@ | |||
| local github = dofile("apis/github") | |||
| local passed = {...} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this slipped through the last one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats wrong with this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see what's wrong - it's repeated further down! (line 62?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah derp... had to move it to top of file to avoid stupid vararg function error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reload the latest PR
programs/github
Outdated
|
|
||
| local usage = "Usage: github clone <user>/<repo> [<branch>|<tag>] [<destination>]" | ||
| local validActions = { | ||
| clone = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line isn't a tab!
|
check out the latest... gotta break for a few hours, will continue testing later tonight. |
eric-wieser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good other than the whitespace breakage
programs/github
Outdated
| }, | ||
| auth = { | ||
| }, | ||
| auth = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra tab!
programs/github
Outdated
| @@ -1,10 +1,10 @@ | |||
| local github = dofile("apis/github") | |||
| local passed = {...} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be {select(2, ...)}, so as to automatically skip selectAction, which lets you remove the suspect condition I comment on below
programs/github
Outdated
|
|
||
| local function findFlag(arg) | ||
| local acceptedFlags = getFlagTable() | ||
| local acceptedFlags = action.flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
action.flags is shorter than acceptedFlags ;)
programs/github
Outdated
| end | ||
|
|
||
| if action == 'clone' then | ||
| if action.name == 'Clone' then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue if action == validActions.clone is better (and that validActions is better named subcommands)
programs/github
Outdated
| size = size + item.size | ||
| term.clearLine() | ||
| print(" "..item:fullPath()) | ||
| print(" "..item:fullPath()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finding and replacing space pairs with tabs was not the right solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fml
| else | ||
| print(validActions[action].usage) | ||
| end | ||
| print(action.usage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this handle the case where we fail to find a valid action? Or is that handled above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is handled above now... We shouldn't get here unless action is defined and valid, but missused for some reason.
apis/github
Outdated
| end | ||
| Auth.get = function(user) | ||
| local authTable = Auth.getAll() | ||
| return authTable[user] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spaces here should be tabs
readme.md
Outdated
| ----- | ||
|
|
||
| github clone <user>/<repo> [<branch>|<tag>] [<destination>] | ||
| #### Cloning a repo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be three not four hash signs
|
@eric-wieser tested, rebased, and ready for deep code-review |
aaronmallen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eric-wieser I did a little CR of my own but will wait for yours before pushing these changes.
readme.md
Outdated
|
|
||
| github auth <user> [<api token> | -d] | ||
|
|
||
| The delete argument is optional and will delete specified user when defined. The user and token arguments maybe undefined when using the delete option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damnit @eric-wieser this will need to be changed to
The api token argument maybe undefined when using the delete option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue that all of these words are fully described by the [ a | b ] notation which means "a, b, or nothing at all"
programs/github
Outdated
| local usage = "Usage: github clone <user>/<repo> [<branch>|<tag>] [<destination>]" | ||
| local subcommands = { | ||
| clone = { | ||
| name = 'Clone', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hrm the name property on these tables isn't really needed anymore, should I kill it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think kill it
| local action = subcommands[selectedAction] | ||
| if not action then | ||
| for k, v in pairs(subcommands) do | ||
| print(v.usage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if a line break would be appropriate here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends if they wrap on the default console (how wide is it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wraps....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps manually wrapping the usage strings neatly is the right thing to do then? Unless CC screens can be different sizes. Either way, that's a job for another PR.
programs/github
Outdated
|
|
||
| if flags.authed and not auth then | ||
| printError('Unknown User!') | ||
| print(subcommands['auth'].usage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is printing usage necessary here? we're returning the printError on all other cases like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see this case IS slightly different then other instances of us returning printError, in this particular case we are expecting the user to utilize a separate subcommand, and showing the user how to utilize that subcommand. The question is still semi valid though, should we print usage for the auth subcommand here or just return the printError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about Unknown user! Add one with 'git auth'
|
A small caveat to this change: with an authorized request it is now possible to download a repo that exceeds the allowed amount of space on the computercraft computer. Computercraft actually already handles this pretty well IMO. It'll stop the download and throw a |
|
|
||
| -- A class for authorization | ||
| local authFile = '.github-auth' | ||
| local function writeAuth(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see why this should be a free function. Why not just leave it in Auth.save?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DRY... I was calling the same logic in save and delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, missed that it was in delete as well. Sure, this is reasonable.
That's not your problem in this pull request, but thanks for the heads up! Perhaps the command should forecast its size before continuing (in another PR). Is there an api to get remaining space in computercraft? |
|
Two final things:
|
|
I agree it's beyond the scope of this PR but it is a limitation we should be aware of. I think there is in the FS API, specifically this method, however I'm not sure what the "size" property is in the github api (is it bytes as well?) |
added support for authorized request updated readme
|
@eric-wieser so heres what I discovered during my testing... My testing was invalid in the first place :( This was never actually counting as an authenticated request. Take a gander at the latest commit to see how I've fixed that :D Basically I was attempting to clone To answer this question:
After finally hitting my daily limit of unauthorized API requests I tried making a request to clone twbs/bootstrap with user: When I try a request to clone twbs/bootstrap with user: I would argue that checking the token is valid would not be within scope of this PR, but I intend to open up a new PR to resolve both this issue AND checking size before downloading :D |
This PR should fail gracefully if the token is wrong, and the request is rejected. If it's not possible to tell the difference between "this worked because I am under the limit", and "this worked because I am authenticated, then sure, leave that for another time |
Fixed
Added
resolves #8