Verify if get_mtime_and_size can take both Path objects and strings

[algomaster99]

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

---

With reference to #2137 and #2673 (comment)

[efiop]

@algomaster99 Please keep an eye on tests, they will tell you if anything is very wrong before I am able to come in and take a look 🙂

[efiop]

The tests are failing because get_mtime_and_size is not converting things properly right now. For example https://github.com/iterative/dvc/blob/0.66.2/dvc/utils/fs.py#L24 https://github.com/iterative/dvc/blob/0.66.2/dvc/utils/fs.py#L42 etc.

[Suor]

Before verifying one needs to actually change get_mtime_and_size() to handle PathInfo arguments. The agreed way to do this is wrapping path args with fspath() unconditionally, see how this is done in dvc.system.System methods.

[Suor]

Not sure this test is valuable:

• this is tested via functional tests anyway
• this doesn't test for non-dir argument

I would just drop it.

[algomaster99]

@Suor @efiop How should I make the required changes?

[efiop]

We are verifying that it works the same with str and path_infos and for that the test is suitable and valuable. Let's keep it.

[efiop]

@algomaster99 But this doesn't test for non-dir argument is a valid one(though it is probably meant to say "dir" instead of "non-dir"). Let's also run this test with self.DATA_DIR, so we verify that this works for dirs as well. Make sure that you are not copypasting the code for the new test, but rather use parametrization. E.g. you could make this as a pytest-style test with something like:

@pytest.mark.parametrize("path", ["data_dir", "data"])

You can look around for parametrize examples in our existing tests. 🙂

[efiop]

The agreed way to do this is wrapping path args with fspath() unconditionally, see how this is done in dvc.system.System methods.

@Suor This is not the agreed way. Current patch handles it much better by using fspath_py35 where needed, so next year we can go and remove those easily when py35 reaches end-of-life.

[Suor]

@efiop having one place at the start of the function to coerce paths is more durable solution. If you only pass that to file utils and expect this to work transparently in Python 3.6+ you might use fspath_py35() instead of fspath() there. Even in this case fspath() might be a better choice both for performance and durability reasons:

• will never stringify paths several times
• the whole func body is easier to reason about since it only has to deal with strings
• no future bug possible when you'll pass that path to something other than fs util

We discussed that previously and I had an impression we agreed on that. Maybe that was only about System.* stuff? Anyway all the arguments above apply. One can think about this as a form of defensive programming, which is also good/ok for performance.

P.S. This is general discussion, which should not delay this particular PR. So we may resolve it in any of those ways.

[algomaster99]

@efiop
I had to move this test out of the class otherwise it wasn't able to recognize path argument. It kept throwing NameError: missing 1 required positional argument: 'path' This could be the reason - https://stackoverflow.com/a/35562401/11751642.

[algomaster99]

@efiop DeepSource test is failing. Do you know how can I fix it? The assert statements were already there. 😕

[efiop]

@Suor Good points! In this particular case, I'm not that worried about performance, as double fspath_py35 will only occur for standalone files(as giant directories are handled under first if block), which is not very significant. In terms of future bugs, this function is now fully covered with the unit test, so we are also covered there. We could still do something like path = fspath_py35(path), since it will be easy to get rid of, sure, but the current approach also doesn't look like a deal-breaker. Not quite sold on fspath(path) as it does an unnecessary conversion, even for performance sake(which is insignificant because of the reasons described above), esp since it would probably have to be solved by caching PathInfo.fspath in a general case, so we don't have to micro-manage.

[efiop]

@algomaster99 Take DeepSource's report as a recommendation, not as a requirement. We are still fine-tuning the settings for it.

[efiop]

Thanks!

[Suor]

@efiop I don't worry too much about performance here too. The main point was simplifying code comprehension. Having tests covering things is good, but writing durable code is a basic measure that also works and is cheap. After we all agree on particularities of cause :)

And, BTW, it looks like they use "convert at the function start" approach in stdlib too, see os.walk() for example.