add: warn when recursively adding large files

[ghost]

• Have you followed the guidelines in our
  Contributing document?

• Does your PR affect documented changes or does it add new functionality
  that should be documented? If yes, have you created a PR for
  dvc.org documenting it or at
  least opened an issue for it? If so, please add a link to it.

---

Close #1758

[efiop]

@MrOutis ?

[ghost]

Sorry, @efiop , forgot to push changes

[efiop]

Let's define this config fmt somewhere and use it both in the code itself and in the test. No real need to copypaste it.

[ghost]

@efiop , I think there's no harm in copy pasting constants on the test, at least in this case. 🙂

If we are going to take this approach for every log message that we are testing, we would end with a lot of constants across the code base.

This is different from having a module with constants like the one used for localization/internationalization purposes (strings.xml - android, locale/en.yml - rails, etc.).

[efiop]

@MrOutis But at least you would only have to modify that message in one place. There is no harm in using constants, actually, it is strongly prefered over copypasting.

[ghost]

Agree, @efiop , it is better to use constants than copy pasting to reuse code, but not to reuse code in tests.

It is kind of like a purist perspective, I guess, that you are modifying the interface of an object by adding another attribute just for the sake of testing, otherwise it wouldn't be needed.

I will change it on my patch, but just wanted to share my perspective 🙂

[efiop]

@MrOutis Thanks! 🙂 I agree that sometimes generalization might be too much, but in this case, it is a fairly static big message, so copypasting it would simply bloat our code. Especially since the point of that test is to simply make sure that the message is printed, not to verify the contents of it.

[ghost]

@efiop , if we add the file name to the warning "You are adding a large directory '{target}' recursively," is not static anymore; we would have something like:

# dvc/repo/add.py
WARNING_ADD_LARGE_DIR_RECURSIVELY = (
    "You are adding a large directory '{target}' recursively,"
    " consider tracking it as a whole instead.\n"
    "{purple}HINT:{nc} Remove the generated stage files and then"
    " run {cyan}dvc add {target}{nc}"
)

# ...

        logger.warning(
            WARNING_ADD_LARGE_DIR_RECURSIVELY.format(
                purple=colorama.Fore.MAGENTA,
                cyan=colorama.Fore.CYAN,
                nc=colorama.Style.RESET_ALL,
                target=target,
            )
        )

can we skip it this time 😛 ?

[efiop]

Indeed, when target is interlaced with colours it is not fun to bother around with partial string formatting. Deal 🙂

requested changes already implemented :)