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

Added an enhancement - issue 366 to add a download all if no argument… #371

Merged
merged 4 commits into from
Oct 3, 2023
Merged

Added an enhancement - issue 366 to add a download all if no argument… #371

merged 4 commits into from
Oct 3, 2023

Conversation

rabroldan
Copy link
Contributor

This updated version automatically gets all blob fields from self.datasource.fields and adds them to the list of fields that will be downloaded if the fields argument is empty. As a result, the function will by default download all of the blob's fields if we don't specify any specific fields as parameters.

Adding an if condition to add the code that set the fields

Kindly please rake a look thank you

@kbolashev kbolashev self-requested a review October 3, 2023 14:26
Copy link
Member

@kbolashev kbolashev left a comment

Choose a reason for hiding this comment

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

Looking good! Need to change a thing though.

Also apologies for not writing this anywhere like a CONTRIBUTING.md (we should probably set up one), but we use black to format the code.


# If no fields are specified, include all blob fields from self.datasource.fields
if not fields:
fields = [field.name for field in self.datasource.fields if field.is_blob()]
Copy link
Member

Choose a reason for hiding this comment

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

There is no is_blob() function on the MetadataFieldSchema enum.
You can either add this function, or compare to MetadataFieldType.BLOB in the comprehension

Suggested change
fields = [field.name for field in self.datasource.fields if field.is_blob()]
fields = [field.name for field in self.datasource.fields if field.valueType == MetadataFieldType.BLOB]

Don't forget to import MetadataFieldType, I don't think it's imported in QueryResult.

@rabroldan
Copy link
Contributor Author

hi @kbolashev Kindly please review. I added and chaged the valeu as requested and added an import from dagshub.data_engine.dtypes import MetadataFieldType also reformatted the code using python black with the added codes into it.

@kbolashev
Copy link
Member

kbolashev commented Oct 3, 2023

Thank you so much @rabroldan !
Ironing out some stuff still, can I ask you to rerun black with -l 120?

I'll make sure to add it to pyproject.toml, and also clear it up in the whole repo so it's consistent.
I'll merge into main in a bit a consistently reformatted repo and you can merge it into your branch.

Really sorry for the last minute inconenience :P

@kbolashev
Copy link
Member

Ok I reran it and resolved the merge conflict, should be able to merge it now.
Thank you for the contribution!

@rabroldan
Copy link
Contributor Author

hey @kbolashev i jsut saw the request. but it seemed you already did it, thank you for allowing me to contribute on this project. i'll keep an eye on some of the issue and see if i can get a few more done.

@kbolashev kbolashev merged commit 93d1a1a into DagsHub:master Oct 3, 2023
5 checks passed
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.

None yet

2 participants