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

dvc: use HashInfo #4495

Merged
merged 2 commits into from
Aug 30, 2020
Merged

dvc: use HashInfo #4495

merged 2 commits into from
Aug 30, 2020

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Aug 30, 2020

Initial implementation of HashInfo to avoid using explicit tuples and have a handier way of working with hashes (e.g. comparing hashes of different types).

Related to #4144 , #3069 , #1676

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

dvc/tree/s3.py Outdated Show resolved Hide resolved


@dataclass
class HashInfo:
Copy link
Member

Choose a reason for hiding this comment

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

Loved this. I was having a lot of issues related to hashes when working with subrepos, mostly my fault but would have made debugging way easier.

Copy link
Member

Choose a reason for hiding this comment

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

Also, a few propertys would make it even better. Eg: is_dir_checksum? But looks like it's not needed right now.

Copy link
Member

@skshetry skshetry Aug 30, 2020

Choose a reason for hiding this comment

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

We should try doing this more in the internals instead of plain dict/tuples/list. Not saying we should use dataclass, but we can use typing.NamedTuple/typing.TypedDict/dataclasses/objects. 🙂

The one that I can remember right now is StatusInfo. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, a few propertys would make it even better. Eg: is_dir_checksum? But looks like it's not needed right now.

Exactly! 🙂 Yeah, just don't want to push everything in this PR. Plus is_dir actually will require some stuff on index/tree side, so don't want to mix those up. Clearly it will be implemented in the near future though 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should try doing this more in the internals instead of plain dict/tuples/list. Not saying we should use dataclass, but we can use typing.NamedTuple/typing.TypedDict/dataclasses/objects. slightly_smiling_face

Sorry, didn't get your point.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, didn't get your point.

Just trying to echo your point, that we should do this more instead of using plain dict/tuples. ⬇️

avoid using explicit tuples

Co-authored-by: Saugat Pachhai <[email protected]>
@efiop efiop merged commit 1a35128 into iterative:master Aug 30, 2020
@efiop efiop added the refactoring Factoring and re-factoring label Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Factoring and re-factoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants