checkout: fix output message#3297
Conversation
@skshetry but is there any particular reason why |
First of all, I tried to maintain consistency: |
| unused = repo.state.get_unused_links(used) | ||
| for link in unused: | ||
| logger.info( | ||
| "Removing '{}' as it already exists in the current worktree.", link |
There was a problem hiding this comment.
cc @jorgeorpinel - please review the messages in this PR
There was a problem hiding this comment.
I couldn't think of succinct messages. 😢
There was a problem hiding this comment.
The message is not very useful for info, IMHO. How about we move it to debug?
There was a problem hiding this comment.
Also, it is pretty weird to split old remove_unused_links just to print a message here. Or am I missing something?
There was a problem hiding this comment.
Also, it is pretty weird to split old
remove_unused_linksjust to print a message here. Or am I missing something?
yes: #3297 (comment)
There was a problem hiding this comment.
First of all, I tried to maintain consistency: state does not have logger.info anywhere. Therefore, I did not try to introduce one, because I wanted to separate concerns (ui vs state/model) wherever possible, and it seemed like a good idea here for doing just that. slightly_smiling_face
So you moved it just to not use logger.info in state? That is a pretty weak argument for doing that, tbh 🙂 There is nothing inherently wrong with using logging.info in state by itself. I think you just made half a step at separating the context and you really felt that "storing" and "removing" should be handled separately. Please see #3297 (comment) , I think that it should be like that or we should keep remove_unused_links in one piece. Let me know what you think.
There was a problem hiding this comment.
I don't consider separation of UI and business logic to be a weak argument. But probably as it's a cli app and already done this way on most of the places here, maybe it is.
I think that it should be like that or we should keep remove_unused_links in one piece. Let me know what you think.
I am fine, either way. Previously, I did not want to separate those too far off like you suggested below and tried to just get the stuff done. 🙈
It does not really need to be separated if logger.info call is kept on dvc.state as it's only used in a single place.
There was a problem hiding this comment.
cc @jorgeorpinel - please review the messages in this PR
I see #3326 replaced this and only has a debug string change
| logger.info( | ||
| "The file '{}' already exists. It will be replaced.", path_info | ||
| ) |
There was a problem hiding this comment.
Not sure about converting it to info. Maybe let's remove it for good? Or make it debug?
There was a problem hiding this comment.
This was the result of the discussion from #2979, no?
Also: #2979 (comment)
There was a problem hiding this comment.
@skshetry Good point, that needs to be clarified. Left a comment there.
| for link in links: | ||
| remove(os.path.join(self.root_dir, link)) |
There was a problem hiding this comment.
A more natural refactor would split old remove_unused_links into get_unused_links and remove_link(s)(as a reverse save_link). So we would get_unused_links, remove them in checkout and then state.remove_links() what we've deleted. That way the context would be properly spread across these parts of code.
|
Closing this PR in favor of #3326. Other summary outputs need discussions, and will follow later on. |
5 participants
❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Fixes #2979.
I tried adding a better message, but could not think any better message. Suggestions will be appreciated.
I also did not like to have
logger.infoindvc.state, so it needed a refactor. 🙂