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

enable and document basic list call from python library #170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

enable and document basic list call from python library #170

wants to merge 1 commit into from

Conversation

axfelix
Copy link

@axfelix axfelix commented Sep 12, 2019

Hi,

We wanted to use this from within a Python script and found that those methods weren't really documented or designed that way, so I've added a basic method and example for doing that.

@codecov-io
Copy link

Codecov Report

Merging #170 into master will decrease coverage by 0.14%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
- Coverage   92.17%   92.03%   -0.15%     
==========================================
  Files          13       13              
  Lines         588      590       +2     
==========================================
+ Hits          542      543       +1     
- Misses         46       47       +1
Impacted Files Coverage Δ
osfclient/cli.py 96.96% <66.66%> (-0.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44b9a87...91976d0. Read the comment docs.

@benlindsay
Copy link
Collaborator

I like the idea of showing how to do it from python in the docs. However, I'm not sure we want to encourage people to put passwords in plain text in python scripts. I'm no security expert though. Also, the [project] notation in the python script might be a little confusing since it looks like a python list. I might recommend something like "myusername" instead of [username] and "xyz123" or "myprojectid" instead of [project].

@axfelix
Copy link
Author

axfelix commented Sep 12, 2019

Yeah, obviously it'd be preferable if you could use a token there instead, but I wanted there to be some way of running it unsupervised -- obviously password could be passed as input from some other part of a script. Happy to make the other changes though.

@benlindsay
Copy link
Collaborator

Doesn't it still work without passing a password from python? Either by setting OSF_PASSWORD or by manually typing it in once OSFClient gets to this line: password = getpass.getpass('Please input your password: ')?

@axfelix
Copy link
Author

axfelix commented Sep 12, 2019

Yeah, the environment variable method still works either way, it just didn't strike me as super pythonic.

@benlindsay
Copy link
Collaborator

Hmm...what seems un-pythonic about it to you? Have you seen examples of high-quality python code that includes plaintext passwords? Putting passwords and keys in environment variables is a pretty common thing to do from what I've seen, even with python projects.

@axfelix
Copy link
Author

axfelix commented Sep 12, 2019

Well, again, it just makes it more explicit this way if you wanted to pass input from elsewhere, since token auth isn't an option. But I'm happy to retract the change to cli.py and relabel the readme changes if you just want to merge those.

@benlindsay
Copy link
Collaborator

Gotcha. I guess I'm just not comfortable enough to make those changes to cli.py at the moment. I don't know enough about possible side effects and I don't have the bandwidth to get there. But if you retract the changes to cli.py, remove the password stuff, and change the [...] stuff, then I'd merge it.

@axfelix
Copy link
Author

axfelix commented Sep 12, 2019

Another issue is that my example doesn't seem to work without setting a password :)

$ OSF_PASSWORD=password
$ python
Python 3.5.2 (v3.5.2:4def2a2901a5, Jun 25 2016, 22:18:55) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import osfclient
>>> import osfclient.cli
>>> osf_login = osfclient.OSF(username="garnett@sfu.ca")
>>> osf_login.project="9qkmd"
>>> osfclient.cli.list_(osf_login)
Please set a username (run `osf -h` for details).

I may have to return to this a bit later.

@benlindsay
Copy link
Collaborator

Yo, you should change your password ASAP. I just logged in (and back out immediately) with your credentials.

@axfelix
Copy link
Author

axfelix commented Sep 12, 2019

Yeah, I know, lazy paste :)

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.

None yet

3 participants