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

addurls: Fix handling of relative URL-FILE #3582

Merged
merged 7 commits into from Aug 6, 2019

Conversation

@kyleam
Copy link
Member

commented Aug 2, 2019

Follow conventions (different on master and 0.11.x) for taking URL-FILE as relative to the specified dataset.

This PR is against master, but it has a 0.11.x component. I've pushed that 0.11.x-based branch to a scratch branch so that we can check its Travis build. If all looks good, I'll merge the 0.11.x-bits to 0.11.x after this PR is merged to master.

Closes #3580.

kyleam added 7 commits Aug 2, 2019
RF: addurls: Follow dataset/ds convention
A good number of commands use `dataset` for the dataset, as provided,
and `ds` for the resolved dataset.  Do the same here.
DOC: addurls: Clarify what filename format is relative to
This deviates a bit from our command-line handling (both on 0.11.x and
master), but I think that is fine given that (1) the files aren't
specified on the command-line, (2) it leads to simpler behavior and
code, and (3) the user can still easily specify desired sudirectories
in the filename format.
BF: addurls: Fix handling of relative paths
Pass the input file to resolve_path() so that we follow the rules for
resolving relative paths against the dataset.  Note that this should
be switched to rev_resolve_path() once this is merged to master.

Closes #3580.
TST: addurls: Drop unneeded chpwd() calls
In addition to being unnecessary, using chpwd() could mask path
handling issues with bound-dataset calls.  Note that aside from the
removed chpwd() calls and some "relative -> absolute" changes in the
test paths, the diff here is purely from indentation.
TST: addurls: Fix indentation of cfg_proc assertion
When this was added in cc28fd7 (ENH: addurls: Add --cfg-proc option
that is passed to create(), 2019-07-24), the intention was to assert
this for _every_ subds.
Merge branch 'addurls-relative-input-maint'
This is admittedly a very ugly merge for test_addurls.py due to the
widespread indentation changes coming in.  The best view of these
changes probably comes from diffing the merge with the second parent:

  $ git diff HEAD^2.. -- datalad/plugin/tests/test_addurls.py

That set of changes can be compared to what would be expected for the
current 0.11.x and master differences:

  $ git diff origin/0.11.x..master -- datalad/plugin/tests/test_addurls.py
RF: addurls: Adjust relative path resolution for new convention
We just merged in the maintenance line fix for taking the input file
relative to the specified dataset, but on master this should use
rev_resolve_path() so that the path considered relative to the dataset
only if it's an _instance_.

Re: #3580

@kyleam kyleam force-pushed the kyleam:addurls-relative-input branch from ebfd2f3 to cdfca01 Aug 2, 2019

@codecov

This comment has been minimized.

Copy link

commented Aug 2, 2019

Codecov Report

Merging #3582 into master will increase coverage by 0.39%.
The diff coverage is 85.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3582      +/-   ##
==========================================
+ Coverage   76.99%   77.39%   +0.39%     
==========================================
  Files         272      255      -17     
  Lines       35354    33676    -1678     
==========================================
- Hits        27222    26063    -1159     
+ Misses       8132     7613     -519
Impacted Files Coverage Δ
datalad/plugin/tests/test_addurls.py 100% <100%> (ø) ⬆️
datalad/plugin/addurls.py 17.88% <7.69%> (+0.24%) ⬆️
datalad/interface/run.py 67.66% <0%> (-32.34%) ⬇️
datalad/distribution/subdatasets.py 78.03% <0%> (-21.97%) ⬇️
datalad/interface/diff.py 83.12% <0%> (-12.5%) ⬇️
datalad/interface/unlock.py 34.56% <0%> (-9.1%) ⬇️
datalad/interface/utils.py 76.62% <0%> (-8.93%) ⬇️
datalad/support/path.py 66% <0%> (-6.84%) ⬇️
datalad/support/tests/test_path.py 88% <0%> (-4.31%) ⬇️
datalad/support/gitrepo.py 63.32% <0%> (-4%) ⬇️
... and 103 more

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 f833c28...cdfca01. Read the comment docs.

1 similar comment
@codecov

This comment has been minimized.

Copy link

commented Aug 2, 2019

Codecov Report

Merging #3582 into master will increase coverage by 0.39%.
The diff coverage is 85.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3582      +/-   ##
==========================================
+ Coverage   76.99%   77.39%   +0.39%     
==========================================
  Files         272      255      -17     
  Lines       35354    33676    -1678     
==========================================
- Hits        27222    26063    -1159     
+ Misses       8132     7613     -519
Impacted Files Coverage Δ
datalad/plugin/tests/test_addurls.py 100% <100%> (ø) ⬆️
datalad/plugin/addurls.py 17.88% <7.69%> (+0.24%) ⬆️
datalad/interface/run.py 67.66% <0%> (-32.34%) ⬇️
datalad/distribution/subdatasets.py 78.03% <0%> (-21.97%) ⬇️
datalad/interface/diff.py 83.12% <0%> (-12.5%) ⬇️
datalad/interface/unlock.py 34.56% <0%> (-9.1%) ⬇️
datalad/interface/utils.py 76.62% <0%> (-8.93%) ⬇️
datalad/support/path.py 66% <0%> (-6.84%) ⬇️
datalad/support/tests/test_path.py 88% <0%> (-4.31%) ⬇️
datalad/support/gitrepo.py 63.32% <0%> (-4%) ⬇️
... and 103 more

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 f833c28...cdfca01. Read the comment docs.

@kyleam

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Travis passed for both branches.

Without this change, ds.addurls(<relative path>, ...) would be pretty surprising given the behavior of other commands, so I'll merge these branches in.

@kyleam kyleam merged commit cdfca01 into datalad:master Aug 6, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kyleam added a commit that referenced this pull request Aug 6, 2019

@kyleam kyleam deleted the kyleam:addurls-relative-input branch Aug 6, 2019

yarikoptic added a commit that referenced this pull request Sep 6, 2019
Merge tag '0.11.7' into debian
0.11.7 (Sep 02, 2019) -- python2-we-still-love-you-but-...

Primarily bugfixes with some optimizations and refactorings.

 Fixes

- [addurls][]
  - now provides better handling when the URL file isn't in the
    expected format.  ([#3579][])
  - always considered a relative file for the URL file argument as
    relative to the current working directory, which goes against the
    convention used by other commands of taking relative paths as
    relative to the dataset argument.  ([#3582][])

- [run-procedure][]
  - hard coded "python" when formatting the command for non-executable
    procedures ending with ".py".  `sys.executable` is now used.
    ([#3624][])
  - failed if arguments needed more complicated quoting than simply
    surrounding the value with double quotes.  This has been resolved
    for systems that support `shlex.quote`, but note that on Windows
    values are left unquoted. ([#3626][])

- [siblings][] now displays an informative error message if a local
  path is given to `--url` but `--name` isn't specified.  ([#3555][])

- [sshrun][], the command DataLad uses for `GIT_SSH_COMMAND`, didn't
  support all the parameters that Git expects it to.  ([#3616][])

- Fixed a number of Unicode py2-compatibility issues. ([#3597][])

 Enhancements and new features

- The [annotate-paths][] helper now caches subdatasets it has seen to
  avoid unnecessary calls.  ([#3570][])

- A repeated configuration query has been dropped from the handling of
  `--proc-pre` and `--proc-post`.  ([#3576][])

- Calls to `git annex find` now use `--in=.` instead of the alias
  `--in=here` to take advantage of an optimization that git-annex (as
  of the current release, 7.20190730) applies only to the
  former. ([#3574][])

- [addurls][] now suggests close matches when the URL or file format
  contains an unknown field.  ([#3594][])

- Shared logic used in the setup.py files of Datalad and its
  extensions has been moved to modules in the _datalad_build_support/
  directory.  ([#3600][])

- Get ready for upcoming git-annex dropping support for direct mode
  ([#3631][])

* tag '0.11.7': (87 commits)
  DOC: Added an entry to changelogn on merged 3631
  ENH: finalizing changelog for 0.11.7
  TST: Update tests for a git-annex without direct mode
  TST: utils: Add decorator that skips when direct mode is unsupported
  ENH: annexrepo: Refuse to initialize in direct mode if unsupported
  ENH: annexrepo: Add check_direct_mode_support method
  BF+TST: Avoid leaking patched git-annex version
  TST+RF: test_annexrepo: Split up a test
  CHANGELOG.md: Second batch for 0.11.7
  TST: run_procedure: Mark test_spaces() as known Windows failure
  TST: run_procedure: Mark test_quoting as known windows failure
  TST: run_procedure: Test more arguments that need quoting
  BF(py2): run_procedure: Avoid encoding error in log message
  TST: add run_procedure test with spaces in file name
  TST/RF: non-hardcoded Python executable
  RF: newline at end of file
  RF: helper instead of conditional
  RF: remove superfluous imports
  BF/TST: remove quoting
  ENH: replace conditionals with helper function
  ...
yarikoptic added a commit that referenced this pull request Sep 6, 2019
Merge tag '0.11.7' into debian
0.11.7 (Sep 02, 2019) -- python2-we-still-love-you-but-...

Primarily bugfixes with some optimizations and refactorings.

 Fixes

- [addurls][]
  - now provides better handling when the URL file isn't in the
    expected format.  ([#3579][])
  - always considered a relative file for the URL file argument as
    relative to the current working directory, which goes against the
    convention used by other commands of taking relative paths as
    relative to the dataset argument.  ([#3582][])

- [run-procedure][]
  - hard coded "python" when formatting the command for non-executable
    procedures ending with ".py".  `sys.executable` is now used.
    ([#3624][])
  - failed if arguments needed more complicated quoting than simply
    surrounding the value with double quotes.  This has been resolved
    for systems that support `shlex.quote`, but note that on Windows
    values are left unquoted. ([#3626][])

- [siblings][] now displays an informative error message if a local
  path is given to `--url` but `--name` isn't specified.  ([#3555][])

- [sshrun][], the command DataLad uses for `GIT_SSH_COMMAND`, didn't
  support all the parameters that Git expects it to.  ([#3616][])

- Fixed a number of Unicode py2-compatibility issues. ([#3597][])

- [download-url][] now will create leading directories of the output path
  if they do not exist ([#3646][])

 Enhancements and new features

- The [annotate-paths][] helper now caches subdatasets it has seen to
  avoid unnecessary calls.  ([#3570][])

- A repeated configuration query has been dropped from the handling of
  `--proc-pre` and `--proc-post`.  ([#3576][])

- Calls to `git annex find` now use `--in=.` instead of the alias
  `--in=here` to take advantage of an optimization that git-annex (as
  of the current release, 7.20190730) applies only to the
  former. ([#3574][])

- [addurls][] now suggests close matches when the URL or file format
  contains an unknown field.  ([#3594][])

- Shared logic used in the setup.py files of Datalad and its
  extensions has been moved to modules in the _datalad_build_support/
  directory.  ([#3600][])

- Get ready for upcoming git-annex dropping support for direct mode
  ([#3631][])

* tag '0.11.7':
  Changelog entry for download-url paths handling
  ENH: downloaders: Ensure directories for target exist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.