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

feat: Added get_public_url method to KeyValueStore #572

Merged
merged 35 commits into from
Oct 22, 2024

Conversation

akshay11298
Copy link
Contributor

@akshay11298 akshay11298 commented Oct 6, 2024

Description

  • Implement get_public_url method in KeyValueStore

Issues

Testing

  • Unit tests added

Checklist

  • CI passed

@janbuchar
Copy link
Collaborator

This looks like the right approach. You should use the storage_dir attribute of the configuration object instead of the current directory though.

@akshay11298
Copy link
Contributor Author

This looks like the right approach. You should use the storage_dir attribute of the configuration object instead of the current directory though.

@janbuchar Thanks for the review and hint :) I have updated the PR with storage_dir

tests/unit/storages/test_key_value_store.py Outdated Show resolved Hide resolved
src/crawlee/storages/_key_value_store.py Outdated Show resolved Hide resolved
src/crawlee/storages/_key_value_store.py Outdated Show resolved Hide resolved
src/crawlee/storages/_key_value_store.py Outdated Show resolved Hide resolved
@fnesveda fnesveda added the t-tooling Issues with this label are in the ownership of the tooling team. label Oct 9, 2024
@janbuchar
Copy link
Collaborator

@akshay11298 sorry for the broken CI. I confirm that code quality checks are passing on my machine.

@akshay11298
Copy link
Contributor Author

akshay11298 commented Oct 20, 2024

@janbuchar @vdusek found the cause of the error. It was the slash used when creating the path manually. Used os.path.join to fix the error

Edit:
Just saw the build result, the test is not able to read the file. Its giving permission denied error. Although I am wondering if this was created by the application, ideally it should have the access as well. I will be checking this. Although any idea on why it may occur though?

Akshay Avinash added 2 commits October 22, 2024 06:25
@akshay11298
Copy link
Contributor Author

akshay11298 commented Oct 22, 2024

@janbuchar @vdusek Now the test passes. urlparse was returning netloc in windows and path in mac/linux

Windows -
image

Mac -
image

Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

Thanks, @akshay11298, for solving it. I added one more test, test_get_public_url_raises_for_non_existing_key, and updated the method a bit so that it passes. LGTM.

@vdusek vdusek requested a review from janbuchar October 22, 2024 07:20
Copy link
Collaborator

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

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

LGTM

@vdusek vdusek merged commit 3a4ba8f into apify:master Oct 22, 2024
19 checks passed
@akshay11298 akshay11298 deleted the kvstore-public-url branch October 22, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement KeyValueStore.get_public_url
4 participants