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

RF: <Dataset path=...> -> <Dataset ...> #4420

Merged
merged 1 commit into from Apr 19, 2020
Merged

RF: <Dataset path=...> -> <Dataset ...> #4420

merged 1 commit into from Apr 19, 2020

Conversation

mih
Copy link
Member

@mih mih commented Apr 17, 2020

repr of a dataset always uses the 'path=' prefix to the path, but a
dataset is also always and only identified by a path. Hence the prefix
is superfluous, and only takes space. On any platform an absolute path
is easy to recognize as such.

This change removes the 'path=' prefix.

kyleam
kyleam approved these changes Apr 17, 2020
Copy link
Collaborator

@kyleam kyleam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

@yarikoptic
Copy link
Member

YES, PLEASE! ;) I always hated it but was too shy to suggest stripping it away -- one pros will be that you could just double click on the path to select it in the terminal, where now it would also grab path= prefix

@yarikoptic
Copy link
Member

so, not a single unittest tested our repr for datasets? hm...

@kyleam
Copy link
Collaborator

kyleam commented Apr 17, 2020

one pros will be that you could just double click on the path to select it in the terminal

I'm looking forward to that too :) In the debugger, I often type <dataset>.path so that I can get something I can double-click on.

@kyleam
Copy link
Collaborator

kyleam commented Apr 17, 2020

I'd be happy to see this change land as is, but even better to my eyes would be a format like Dataset('/path/to/ds'), which I probably prefer just because it is a more common format for __repr__s.

@codecov
Copy link

codecov bot commented Apr 17, 2020

Codecov Report

Merging #4420 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4420   +/-   ##
=======================================
  Coverage   88.91%   88.92%           
=======================================
  Files         285      285           
  Lines       37765    37802   +37     
=======================================
+ Hits        33578    33614   +36     
- Misses       4187     4188    +1     
Impacted Files Coverage Δ
datalad/distribution/dataset.py 94.14% <100.00%> (ø)
datalad/support/tests/test_annexrepo.py 95.29% <0.00%> (-0.03%) ⬇️
datalad/interface/tests/test_utils.py 100.00% <0.00%> (ø)
datalad/interface/utils.py 94.11% <0.00%> (+0.02%) ⬆️
datalad/core/distributed/tests/test_clone.py 92.95% <0.00%> (+0.21%) ⬆️

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 23d7683...226bee8. Read the comment docs.

@mih
Copy link
Member Author

mih commented Apr 18, 2020

So what about this then (to really get to a C&P-able state that works for the cmdline and for Python):

>>> print('Dataset({})'.format(quote_cmdlinearg("/home/mih/this is fun/dataset")))       
Dataset('/home/mih/this is fun/dataset')

>>> print('Dataset({})'.format(quote_cmdlinearg("/home/mih/that's it folks/dataset")))   
Dataset('/home/mih/that'"'"'s it folks/dataset')

@kyleam
Copy link
Collaborator

kyleam commented Apr 18, 2020

Looks nice to me.

@yarikoptic
Copy link
Member

Yes!

__repr__ of a dataset always uses the 'path=' prefix to the path, but a
dataset is also always and only identified by a path. Hence the prefix
is superfluous, and only takes space. On any platform an absolute path
is easy to recognize as such.

This change switched the formatting to yield a C&P ready path, with
all needed quoting for the cmdline.
@mih
Copy link
Member Author

mih commented Apr 18, 2020

Force-pushed this change.

@mih mih merged commit 0f4112e into datalad:master Apr 19, 2020
@mih mih deleted the rf-dsrepr branch April 19, 2020 11:18
@yarikoptic yarikoptic added this to the 0.13.0 milestone Apr 23, 2020
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