sympy / sympy-bot-old

SymPy pull request helper
http://reviews.sympy.org/
Other
24 stars 16 forks source link

Authenticated API requests #153

Closed vramana closed 9 years ago

vramana commented 11 years ago

Added _load_conf_file and _load_token in utils/github.py. So, _query makes authenticated requests using the added functions.

asmeurer commented 11 years ago

It would be cleaner to just add a function to get this information and then call it from _query, rather than adding more parameters everywhere.

Also, this should happen automatically. I wouldn't even add command line options.

vramana commented 11 years ago

@asmeurer Okay I'll modify the code but if I am going to write such a function Can I remove --user, --token, --token-file from sympy-bot review also??

asmeurer commented 11 years ago

Ah, I didn't notice those. No, I guess your adding of those arguments is correct then.

vramana commented 11 years ago

But whatever you told makes sense. Instead of all these parameter we can maintain a variable auth in all the functions which will tell whether there is a config file containing token. If the config file is not present we can print a line "Making unauthorised requests" at the start and continue.. If the config file is present, then we will just pass this variable instead of username, password and token in all the functions of github.py and _query will load config file and make authenticated requests. What do you think about this??

asmeurer commented 11 years ago

I still think a global value would be better.

vramana commented 11 years ago

@asmeurer Can you explain in what sense a global value would be better? Are saying that username, token, should be global in sympy-bot or somewhere?

asmeurer commented 11 years ago

Yes. There should just be some function in utils somewhere that returns them. That function will handle getting them and storing them across the session. Then _query will just call get_token or whatever you call it to get the token.

vramana commented 11 years ago

@asmeurer Can please review this??

vramana commented 11 years ago

@asmeurer Can we use :white_check_mark: (white check mark) instead of :eight_spoked_asterisk: (eight spoked asterisk) in sympy-bot?

asmeurer commented 11 years ago

Probably. Open a new pr for it.

asmeurer commented 11 years ago

Any progress on this? Those functions need to be deduplicated.

vramana commented 11 years ago

Sorry I was little busy. Where should keep these here? Should I keep these here and delete the ones sympy-bot and import these into sympy-bot?? I think they may fit into cmd.py as well.

asmeurer commented 11 years ago

Should I keep these here and delete the ones sympy-bot and import these into sympy-bot??

Yes.

vramana commented 11 years ago

@asmeurer Can you review this PR?? I cleaned up the code.

vramana commented 11 years ago

@flacjacket Can you review this PR?

flacjacket commented 11 years ago

It looks like each time you need the token, you re-parse the config file and get the token information. Instead of this, as Aaron suggested, you should create functions that store the username/token when the configuration is first loaded, then has functions to return these values. It does make sense to have separate functions to do this, but you don't want them to keep re-reading the configuration file.

vramana commented 11 years ago

I agree with whatever you have said. It's only in github_list_pull_requests in which _query is called very often rest others are called atmost once or twice per session. If github_list_pull_requests reads the conf file and then send username and token as optional arguments to _query then it doesn't have to re-parse the conf file. Will it be fine?

vramana commented 11 years ago

A small doubt. If I create two global variables conf_username and conf_token in github.py which will parse conf file stores username and token. Will the conf file be parsed once or multiple times when _query is repeatedly called in github_list_pull_requests?