pull, at import stage: replace warning with info report #2693
Conversation
sadielbartholomew
force-pushed
the
i2685-pull-cmd-output
branch
from
2b474e0 to
b2c64b3
Compare
|
@sadielbartholomew thanks! let's for now just start with something simple like For the warning itlsef. It looks like it's being used in |
|
Thanks for the clarification @shcheklein, it makes it easier to know more specifically what you want to resolve the Issue.
Sure, that makes sense. I read through #2667 as referenced in the Issue but was not sure what sort of "stats" would be useful. I'll squash in a new commit to change the report according to your requests above, & using those three stats you suggest. I can always add more afterwards if you think it would help.
Yes, the warning does emerge from the call to
So, you prefer to just remove the warning from |
@efiop could you chime in and shed some light why do we show that warning even though checkout is happening nonetheless. |
Change messages output after return of some commands: * Remove warning for 'dvc checkout' & commands calling it under the hood * Add an informational message output for 'dvc pull' command Fixes treeverse#2685.
sadielbartholomew
force-pushed
the
i2685-pull-cmd-output
branch
from
b2c64b3 to
31551e6
Compare
dvc/repo/pull.py
Outdated
| recursive=recursive, | ||
|
|
||
| ) | ||
| logger.info("Data retrieved successfully from DVC remote storage.") |
There was a problem hiding this comment.
qq - can we analyze results of self._fetch (processed_files_count in this case) and self._checkout to show a different message if nothing has changed or there are no files to process?
There was a problem hiding this comment.
Yes, sure. Sorry I was delayed in writing up the message below to say I will start adding in the stats information tomorrow, as don't have time tonight but wanted to address the first set of comments to being with. Thanks again for your feedback.
shcheklein
left a comment
shcheklein
left a comment
There was a problem hiding this comment.
looks great! thanks @sadielbartholomew . Just one minor question to address.
|
I've just reverted the PR proposed code changes to the state where the warning has been removed for |
|
@sadielbartholomew absolutely! and thank for doing this 🙏 |
|
@sadielbartholomew Sorry for jumping in, but I've decided to adjust your PR a bit to get that warning removal quickly merged :) Please open a new PR for #2685 Also, I've noticed that you had your patch formatting broken, please refer to our contrib guide and install pre-commit hooks so they handle that automatically. Thanks! 🙂 |
No problem, thanks. Also, sorry, if I had known there was need to get the warning removed quickly I would have prioritised this over my other (non-work) dev.
Sure, I'll heed that advice & try to get the PR in shortly (though I might leave it until next week now there is less rush, without an open PR taking up reviewer time & effort). |
|
@sadielbartholomew No rush at all! Thank you so much for contributing! 🙏 |
|
No problems, thanks for being so helpful & welcoming :) |
3 participants
Replace generic locked checkout warning with an information report on return when 'dvc pull' is run at import stage.
Related to #2685.
Have you followed the guidelines in our Contributing document?
Does your PR affect documented changes or does it add new functionality[It does not for both cases.]that should be documented?
The new output, after the minimal steps to recreate as outlined in the Issue to close, is: