Skip to content

Update graphie files to focus on https only downloads. - #572

Merged
rtibbles merged 1 commit into
learningequality:developfrom
rtibbles:graphie_https_only
Feb 18, 2025
Merged

Update graphie files to focus on https only downloads.#572
rtibbles merged 1 commit into
learningequality:developfrom
rtibbles:graphie_https_only

Conversation

@rtibbles

Copy link
Copy Markdown
Member

Summary

  • Updates graphie tests to focus on downloading from https, rather than local file paths

Fix original_filename generation on windows.

@marcellamaki marcellamaki left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers but just some clarifying questions.

Also wondering - is there any utility to having tests on a different test path? beyond something like the pattern:
https://example.com/graphie/test-graph

I am not nearly familiar enough with graphie files to know if having test cases for any variations on this is worthwhile. (Beyond with the .svg and .json file types I mean. Different or potentially more complex? path structure)


def __init__(self, path, **kwargs):
self.original_filename = path.split(os.path.sep)[-1].split(".")[0]
self.original_filename = path.split("/")[-1].split(".")[0]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So here, we are changing this because it is going to always be on a remote URL, not a local file path, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct, and so on windows, this actually breaks things, because os.path.sep is \ there.

Comment thread tests/test_files.py
return

# Yield content in chunks of specified size
for i in range(0, len(content), chunk_size):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand which scenarios we would actually be passing in a (test?) chunk_size here. I think this is just in different files? i.e. chefs.py https://github.com/marcellamaki/ricecooker/blob/develop/ricecooker/chefs.py#L859

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The content is always going to be smaller than the chunk size - I just wanted to make sure we were iterating over it, just in case we wanted to make it bigger at some point.

@rtibbles

Copy link
Copy Markdown
Member Author

Also wondering - is there any utility to having tests on a different test path? beyond something like the pattern:

You'll not that I do use different paths for some tests - specifically to side step the caching that happens in other tests.

@rtibbles
rtibbles merged commit d6bd16a into learningequality:develop Feb 18, 2025
@rtibbles
rtibbles deleted the graphie_https_only branch February 18, 2025 22:38
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.

2 participants