Skip to content

Conversation

@skshetry
Copy link
Collaborator

@skshetry skshetry commented Nov 21, 2019

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

Description

Resolves #2823

I did not feel brave enough on dvc.remote.s3.RemoteS3.exists.
EDIT: dvc.remote.s3.RemoteS3.exists cannot be changed, as path_isin is os-agnostic, whereas s3 requires posixpath. dvc/utils/__init__.py:260 has a cyclic dependency problem.

There are few other cases of startswith, but are not related with filepaths (except for dvc/utils/__init__.py at the end ⏬)

➜ rg -i startswith dvc tests
dvc/prompt.py
48:    return answer and answer.startswith("y")
dvc/remote/http.py
76:        if etag.startswith("W/"):
dvc/remote/config.py
97:        if remote == RemoteLOCAL and not url.startswith("remote://"):
tests/func/test_repro.py
1076:        assert i.startswith(prefix) and o.startswith(prefix)
1588:    elif line.startswith("cmd: "):
dvc/utils/fs.py
147:    return child != parent and child.startswith(parent)
dvc/utils/__init__.py
260:            while parts[0].startswith(plugin_bin):

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

@skshetry skshetry force-pushed the refactor-startswith-pathisin branch from d391414 to 751bdb2 Compare November 21, 2019 16:37
@skshetry skshetry marked this pull request as ready for review November 21, 2019 17:05
@shcheklein shcheklein requested review from a user and pared November 21, 2019 17:19
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

thanks a lot, @skshetry !

@ghost
Copy link

ghost commented Nov 21, 2019

@skshetry , for S3 we use posixpath, if path_isin works for both POSIX and Windows, you can substitute it.

When working with CloudInfoURL you need to use .path to retrieve the path from the whole URL, for example: https://example.com/foo.path -> foo.

@skshetry
Copy link
Collaborator Author

skshetry commented Nov 21, 2019

@MrOutis, yes, path_isin supports both kind of paths. I'll push a change for s3 in a different commit, and you can give me feedback, if it's correct.

@skshetry skshetry requested a review from a user November 21, 2019 18:13
@skshetry skshetry force-pushed the refactor-startswith-pathisin branch from 7472dad to 751bdb2 Compare November 21, 2019 18:51
@skshetry skshetry requested a review from a user November 21, 2019 19:04
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.

Nicely done, thank you!

@efiop efiop merged commit 3de46dd into treeverse:master Nov 22, 2019
@skshetry skshetry deleted the refactor-startswith-pathisin branch November 22, 2019 11:57
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.

Use path_isin instead of startswith to compare filepaths

3 participants