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

BF: gitrepo: Yield 'add' result when saving new submodule #3398

Merged
merged 1 commit into from May 14, 2019

Conversation

@kyleam
Copy link
Contributor

@kyleam kyleam commented May 10, 2019

When rev-save saves a fresh subdataset, it doesn't show any
information about adding that subdataset:

$ datalad create subds
$ datalad rev-save
add(ok): .gitmodules (file)  # <- no subdataset mentioned here...
save(ok): . (dataset)
action summary:
  add (ok: 1)                # <- ... or here.
  save (ok: 1)

Yield a result so that the output is instead:

$ datalad rev-save
add(ok): subds (file)        # <- new entry
add(ok): .gitmodules (file)
save(ok): . (dataset)
action summary:
  add (ok: 2)
  save (ok: 1)

The new result is consistent with the result that is yielded when
saving modified subdatasets.

When rev-save saves a fresh subdataset, it doesn't show any
information about adding that subdataset:

    $ datalad create subds
    $ datalad rev-save
    add(ok): .gitmodules (file)  # <- no subdataset mentioned here...
    save(ok): . (dataset)
    action summary:
      add (ok: 1)                # <- ... or here.
      save (ok: 1)

Yield a result so that the output is instead:

    $ datalad rev-save
    add(ok): subds (file)        # <- new entry
    add(ok): .gitmodules (file)
    save(ok): . (dataset)
    action summary:
      add (ok: 2)
      save (ok: 1)

The new result is consistent with the result that is yielded when
saving modified subdatasets.
@@ -3532,6 +3532,16 @@ def save_(self, message=None, paths=None, _status=None, **kwargs):
else ('not a Git repository: %s', exc_str(e)),
logger=lgr)
continue
# This mirrors the result structure yielded for
# to_stage_submodules below.
yield get_status_dict(
Copy link
Contributor Author

@kyleam kyleam May 10, 2019

I was on the fence about whether this result should match the structure yielded for modified subdatasets or whether it should be something like action='add_submodule', type='dataset', ....

Copy link
Member

@mih mih May 14, 2019

Your choice would have been my preference too.

@codecov
Copy link

@codecov codecov bot commented May 10, 2019

Codecov Report

Merging #3398 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3398      +/-   ##
==========================================
+ Coverage   91.26%   91.26%   +<.01%     
==========================================
  Files         265      265              
  Lines       34360    34362       +2     
==========================================
+ Hits        31360    31362       +2     
  Misses       3000     3000
Impacted Files Coverage Δ
datalad/support/gitrepo.py 89.95% <100%> (ø) ⬆️
datalad/core/local/tests/test_save.py 100% <100%> (ø) ⬆️

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 8b908e6...3dd23bf. Read the comment docs.

refds=self.pathobj,
type='file',
key=None,
path=self.pathobj / ut.PurePosixPath(cand_sm),
Copy link
Member

@mih mih May 14, 2019

There could be places where yield a pathlib instance would be a problem. But I have a long-standing plan to address this at the main result handling loop... ;-)

Copy link
Contributor Author

@kyleam kyleam May 14, 2019

Sounds good (and this shouldn't cause any new problems since other places in gitrepo already yield pathlib objects).

@mih
Copy link
Member

@mih mih commented May 14, 2019

Thx!

@mih mih merged commit 108ca28 into datalad:master May 14, 2019
5 checks passed
@kyleam kyleam deleted the save-add-result branch May 14, 2019
mih added a commit to mih/datalad that referenced this issue Dec 13, 2020
For additional improvement TODOs see the diff.

Importantly, this consolidation makes it obsolete to have the result
record be consistent with the one that is yielded by `add()` (which was
used on submodule update). An unfortunate setup, that was criticized
already a long time ago
datalad#3398 (comment)

Minus the issue of not breaking the promise of result invariance
over time, we are no free to use sensible result records.
mih added a commit to mih/datalad that referenced this issue Dec 15, 2020
For additional improvement TODOs see the diff.

Importantly, this consolidation makes it obsolete to have the result
record be consistent with the one that is yielded by `add()` (which was
used on submodule update). An unfortunate setup, that was criticized
already a long time ago
datalad#3398 (comment)

Minus the issue of not breaking the promise of result invariance
over time, we are no free to use sensible result records.
mih added a commit to mih/datalad that referenced this issue Dec 16, 2020
For additional improvement TODOs see the diff.

Importantly, this consolidation makes it obsolete to have the result
record be consistent with the one that is yielded by `add()` (which was
used on submodule update). An unfortunate setup, that was criticized
already a long time ago
datalad#3398 (comment)

Minus the issue of not breaking the promise of result invariance
over time, we are no free to use sensible result records.
mih added a commit to mih/datalad that referenced this issue Dec 17, 2020
For additional improvement TODOs see the diff.

Importantly, this consolidation makes it obsolete to have the result
record be consistent with the one that is yielded by `add()` (which was
used on submodule update). An unfortunate setup, that was criticized
already a long time ago
datalad#3398 (comment)

Minus the issue of not breaking the promise of result invariance
over time, we are no free to use sensible result records.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants