Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependency not consider dvcignore #5605

Closed
wants to merge 3 commits into from

Conversation

karajan1001
Copy link
Contributor

@karajan1001 karajan1001 commented Mar 11, 2021

fixes #5256

  1. add a new test
  2. get rid of dvcignore in dependency

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

fix#5256

1. add a new test
2. dependency now didn't consider dvcignore
@karajan1001
Copy link
Contributor Author

@efiop
As you had said

For the record: the original issue is not fixed. Having dvcignore built into LocalFileSystem is no longer suitable, esp with fsspec migration. I would consider using it explicitly to check if something is ignored, kinda like git add does, instead of having an implicit behaviour.

This PR is a temporary solution, too many use_dvcignores.

@karajan1001 karajan1001 requested a review from pmrowla April 8, 2021 07:40
@efiop efiop requested review from efiop and removed request for pmrowla April 9, 2021 09:44
@efiop
Copy link
Contributor

efiop commented Apr 12, 2021

@karajan1001 Thanks for clarifying! I was actually about to suggest looking into decoupling dvcignore from filesystems and using it explicitly. Could you please look into that? These days with fsspec aproaching it is even more clear that that needs to be done anyway.

@karajan1001
Copy link
Contributor Author

karajan1001 commented Apr 14, 2021

Waiting for #5812

@efiop efiop mentioned this pull request Apr 15, 2021
10 tasks
@karajan1001
Copy link
Contributor Author

#5265 would be solved automatically after #5812, close this PR.

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.

dependency: don't use dvcignore
2 participants