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

Update client.py #7

Closed
wants to merge 1 commit into from
Closed

Update client.py #7

wants to merge 1 commit into from

Conversation

ntua-el20184
Copy link

No description provided.

@manonthegithub
Copy link

Hey @ntua-el20184 it seems that you are using the ChatGPT for random code generation. Please don't do it in our repos.
I don't see any changes in
__get_extensions and create_dataset functions. the download function was not modified, but just one more function was written so not there are two download functions.
Please check the code before submitting. Please also add tests for all the changes you submit. I will close this PR. Please feel free to open a new one containing tests and real changes to the functions.

@JJ-Author
Copy link
Contributor

JJ-Author commented Feb 12, 2024

@manonthegithub this PR is related to #6, @ntua-el20184 forgot to mention it here but he wrote it in the issue

@JJ-Author JJ-Author reopened this Feb 12, 2024
@manonthegithub
Copy link

@JJ-Author I now that. Please read the comment, it is not because the PR is not linked to an Issue

@JJ-Author
Copy link
Contributor

I agree that tests would be good, but I wanted to test it myself first. did you? also @ntua-el20184 should have the chance to reply to our question and revise his PR. just closing a PR without any chance to do so seems not a polite way, especially since it is my issue and I started reviewing the PR

@manonthegithub
Copy link

manonthegithub commented Feb 12, 2024

@JJ-Author I did the review, it is not from nothing, my comment. I wrote above what is wrong and below you can see more details.
@JJ-Author would be cool if you add some comment or assign yourself to a PR if you started working on it, then we all know that someone has taken it.
Closing PR seems okay when people submit something saying it makes fixes where you see that the code does not correspond to the comments.
They are still free to open a new one with real fixes

for url in urls:
__download_file__(url=url, filename=os.path.join(local_dir, wsha256(url)))

def download(

Choose a reason for hiding this comment

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

the function overrides the download below in the code, it is not editing of the old function function, but writing a new one in the same file with the old one on line 461 in old version

# Changes made to fix the issue:
# 1. __get_content_variants function now correctly extracts content variants from the distribution string.
# 2. __get_extensions function now properly infers file format and compression from the URL path.

Choose a reason for hiding this comment

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

get_extension function is not changed, create_dataset function is not changed, get_content_variants function is not changed

@JJ-Author
Copy link
Contributor

@ntua-el20184 prepped a commit for you to open another PR
d3ca19c
please incorporate changes into the download function where I added TODO comments
and address the issues mentioned from @manonthegithub
especially adding tests to download each one of user, group, artifact, version, file

@@ -25,6 +28,94 @@ class DeployLogLevel(Enum):
info = 1
debug = 2

# Define constants for different identifiers
GROUP_IDENTIFIER = "group"
Copy link
Contributor

Choose a reason for hiding this comment

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

hey thanks for your interest and contribution. will have a deeper look later. but quick question - what are those? the type can actually already be determined by the Databus-identifier/URI see e.g. https://dbpedia.gitbook.io/databus/model/uridesign

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.

3 participants