list: colorize output

[gurobokum]

Close #3347

• ❗ 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. 🙏

[efiop]

@JIoJIaJIu Btw, we've been thinking about --show-json for dvc ls to make it useful in automated scenarios. How about we make repo.ls return a dict/list of dicts that would contain an info dict for each path? That info dict would tell if that path is an output, directory, etc. That info then could be processed and colorized properly in dvc/command/ls.py. And we'll get a cheap dvc ls ... --show-json in the future 🙂

[efiop]

Looks amazing! 🔥

Just a few minor comments left.

[pared]

Looks good! One comment, more of a question, than a suggestion.

[Suor]

A couple of bugs, some small optimization and cosmetics.

[Suor]

If there is no checksum it might still be dir, I would write a note here about this edge case. Now the code looks quirky for no apparent reason.

[gurobokum]

As I got if out.checksum is missed than out.is_dir_checksum fails
Do you have thoughts how can it be improved?

[Suor]

Yeah, it fails, we don't really know whether that is a dir. We may just say about this limitation in a comment.

[gurobokum]

we don't really know whether that is a dir

In what case the out can be dir but doesn't have checksum?

[Suor]

Yes.

[gurobokum]

@Suor it was an open question - could you please provide the case when it can happen?

[Suor]

When a user edits a .dvc file and deletes the output checksum there. This is not as weird as it might sound, people may want to do that to recalc the checksum from the actual dir they have.

[gurobokum]

Ok, I can imagine that user don't see folder when has been working on dvc file and doesn't understand what's going on, but it's really specific case. I can handle it in the #3394. What do you think @efiop?

[skshetry]

Looks good. 🔥 Just a few suggestions.

[skshetry]

Suggested change

	def _prettify(entries, with_color=False):
	if with_color:
	ls_colors = LsColors()
	fmt = ls_colors.format
	else:
	def fmt(entry):
	return entry["path"]
	return [fmt(entry) for entry in entries]
	def _prettify(entries, with_color=False):
	def fmt(entry):
	return entry["path"]
	if with_color:
	ls_colors = LsColors()
	fmt = ls_colors.format
	return [fmt(entry) for entry in entries]

Or, maybe, even use a lambda?

[gurobokum]

I've remembered why I did it as it is
with lambda there is an E371 style error

def _prettify(entries, with_color=False):
    if with_color:
        ls_colors = LsColors()
        fmt = ls_colors.format
    else:
        fmt = lambda entry: entry["path"]

    return [fmt(entry) for entry in entries]

dvc/command/ls/__init__.py:17:5: E731 do not assign a lambda expression, use a def

Unfortunately with you suggestion I am getting F811

def _prettify(entries, with_color=False):
     def fmt(entry):
        return entry["path"]

    if with_color:
        ls_colors = LsColors()
        fmt = ls_colors.format
    return [fmt(entry) for entry in entries]

dvc/command/ls/__init__.py:20:9: F811 redefinition of unused 'fmt' from line 15

[skshetry]

@JIoJIaJIu, It's fine to ignore redefinition warnings (it's what I suggested). But, I'm fine as-is, it's only a suggestion.

[skshetry]

It'd be one less redirection if we just did following on all of the tests:

ls_colors.format({"path": f, "isexec": True})

[gurobokum]

Thank you @skshetry for the point!
The changes came from the comment #3351 (comment) in terms of making tests more readable

[skshetry]

Are other keys mandatory? If they aren't, I don't really think there's any need for redirection.

If they are, you can just:

shape = {
  "isexec": False,
  "isdir": False,
  "isout": False,
}
ls_colors.format({"path": f, **shape, "isexec": True})

But, I'm fine with it as well. But, one less redirection would be great.

[gurobokum]

Are other keys mandatory?

Nope, there are not. In fact before the comment my tests were as you described

I don't see the problem with additional step here (redirection you mean). I think it makes sense not to add additional functionality related to tests only, but let it keep here cause it's thin enough

[efiop]

@JIoJIaJIu @skshetry So what about the changes/questions above?

[gurobokum]

@JIoJIaJIu @skshetry So what about the changes/questions above?

@efiop I've provided my thoughts, I think we can keep the PR in the last shape if you don't mind

[skshetry]

@efiop, I have already approved. It was just a few suggestions. I'm fine as-is.