Skip to content

Conversation

@algomaster99
Copy link
Contributor

  • 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
Testing if get_inode can take both - Path-like objects and strings.

@algomaster99 algomaster99 requested a review from efiop October 26, 2019 18:14
@algomaster99
Copy link
Contributor Author

algomaster99 commented Oct 26, 2019

@efiop
https://github.com/iterative/dvc/blob/1baf020de7c3b0024807f30bc7e9eec39a37948c/dvc/state.py#L376
Does this function accept both ways too?

I thought I would delete https://github.com/iterative/dvc/blob/1baf020de7c3b0024807f30bc7e9eec39a37948c/dvc/state.py#L373 because we were replacing get_inode(path) with get_inode(path_info) but since the above function uses it, I didn't.

@algomaster99
Copy link
Contributor Author

@efiop The DeepSource test failed because I unnecessarily used dvc_repo but it's passed as an argument in one of the functions in test_version.py too but never used so how should I make the test pass?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not mocking anything in this test

Suggested change
# Mocking a file to get the inode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efiop I am creating a file FOO so that I can mock a path. Isn't this called mocking?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mocking is when you simulate something, and here you are testing on a real file, so I wouldn't call that mocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efiop Oh okay. But how does it delete the file later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efiop Can you answer this, please? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@algomaster99 Sorry, missed this. Yes, it deletes it. See dvc_repo fixture in tests/conftest.py.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comments above

@efiop
Copy link
Contributor

efiop commented Oct 27, 2019

Does this function accept both ways too?

Yes, it should accept both str and path_info too, but let's fix it in a separate PR 🙂

. because we were replacing get_inode(path) with get_inode(path_info) but since the above function uses it, I didn't.

Got it. Yes, this PR is mergable by itself so let's finish this and proceed with get_mtime_and_size next, in shich we would be able to remove fspath_py35 from that method in state.py.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Let's remove that comment and we will be ready to merge 🙂

@algomaster99 algomaster99 requested a review from efiop October 27, 2019 08:59
Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! Thanks! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants