ronantakizawa/codereview-bench huggingface dataset:
Repository: apify/crawlee-python
File: src/crawlee/browsers/__init__.py (Python)

Code before:
try:
    from .browser_pool import BrowserPool
    from .playwright_browser_controller import PlaywrightBrowserController
    from .playwright_browser_plugin import PlaywrightBrowserPlugin
except ImportError as exc:
    raise ImportError(
        "To import anything from this subpacakge, you need to install the 'playwright' extra."
        "For example, if you use pip, run `pip install 'crawlee[playwright]'`.",
    ) from exc

__all__ = ['BrowserPool', 'PlaywrightBrowserController', 'PlaywrightBrowserPlugin']


Review comment (suggestion):
```suggestion
        "To import anything from this subpackage, you need to install the 'playwright' extra."
```

Code after:
try:
    from .browser_pool import BrowserPool
    from .playwright_browser_controller import PlaywrightBrowserController
    from .playwright_browser_plugin import PlaywrightBrowserPlugin
except ImportError as exc:
    raise ImportError(
        "To import anything from this subpackage, you need to install the 'playwright' extra."
        "For example, if you use pip, run `pip install 'crawlee[playwright]'`.",
    ) from exc

__all__ = ['BrowserPool', 'PlaywrightBrowserController', 'PlaywrightBrowserPlugin']
Repository: apify/crawlee-python
File: src/crawlee/playwright_crawler/__init__.py (Python)

Code before:
try:
    from .playwright_crawler import PlaywrightCrawler
    from .types import PlaywrightCrawlingContext
except ImportError as exc:
    raise ImportError(
        "To import anything from this subpacakge, you need to install the 'playwright' extra."
        "For example, if you use pip, run `pip install 'crawlee[playwright]'`.",
    ) from exc

__all__ = ['PlaywrightCrawler', 'PlaywrightCrawlingContext']


Review comment (suggestion):
```suggestion
        "To import anything from this subpackage, you need to install the 'playwright' extra."
```

Code after:
try:
    from .playwright_crawler import PlaywrightCrawler
    from .types import PlaywrightCrawlingContext
except ImportError as exc:
    raise ImportError(
        "To import anything from this subpackage, you need to install the 'playwright' extra."
        "For example, if you use pip, run `pip install 'crawlee[playwright]'`.",
    ) from exc

__all__ = ['PlaywrightCrawler', 'PlaywrightCrawlingContext']
Repository: apify/crawlee-python
File: src/crawlee/_utils/urls.py (Python)

Code before:
    """Check if a URL is absolute."""
    return bool(urlparse(url).netloc)


def convert_to_absolute_url(base_url: str, relative_url: str) -> str:
    """Convert a relative URL to an absolute URL using a base URL."""
    return urljoin(base_url, relative_url)


def extract_query_params(url: str) -> dict[str, list[str]]:
    """Extract query parameters from a given URL."""
    url_parsed = urlparse(url)
    return parse_qs(url_parsed.query)


def validate_http_url(value: str | None) -> str | None:
    """Validate the given HTTP URL.

    Raises:
        pydantic.error_wrappers.ValidationError: If the URL is not valid.
    """
    if value is None:
        return None
    adapter = TypeAdapter(AnyHttpUrl)
    adapter.validate_python(value)
    return value


Review comment (suggestion):
```suggestion
_http_url_adapter = TypeAdapter(AnyHttpUrl)

def validate_http_url(value: str | None) -> str | None:
    """Validate the given HTTP URL.

    Raises:
        pydantic.error_wrappers.ValidationError: If the URL is not valid.
    """
    if value is not None:
        _http_url_adapter.validate_python(value)

    return value
```

It is wasteful to make a new `TypeAdapter` every time.

Code after:
    """Check if a URL is absolute."""
    return bool(urlparse(url).netloc)


def convert_to_absolute_url(base_url: str, relative_url: str) -> str:
    """Convert a relative URL to an absolute URL using a base URL."""
    return urljoin(base_url, relative_url)


def extract_query_params(url: str) -> dict[str, list[str]]:
    """Extract query parameters from a given URL."""
    url_parsed = urlparse(url)
    return parse_qs(url_parsed.query)


_http_url_adapter = TypeAdapter(AnyHttpUrl)


def validate_http_url(value: str | None) -> str | None:
    """Validate the given HTTP URL.

    Raises:
        pydantic.error_wrappers.ValidationError: If the URL is not valid.
    """
    if value is not None:
        _http_url_adapter.validate_python(value)

    return value
Repository: apify/crawlee-python
File: tests/unit/http_crawler/test_http_crawler.py (Python)

Code before:
    ids=['curl', 'httpx'],
)
async def test_filling_web_form(http_client_class: type[BaseHttpClient]) -> None:
    http_client = http_client_class()
    crawler = HttpCrawler(http_client=http_client)

    form_data = {
        'custname': 'John Doe',
        'custtel': '1234567890',
        'custemail': 'johndoe@example.com',
        'size': 'large',
        'topping': '["bacon", "cheese", "mushroom"]',
        'delivery': '13:00',
        'comments': 'Please ring the doorbell upon arrival.',
    }

    @crawler.router.default_handler
    async def request_handler(context: HttpCrawlingContext) -> None:
        response = json.loads(context.http_response.read())
        # The httpbin.org/post endpoint returns the form data in the response.
        assert response['form'] == form_data

    request = Request.from_url(
        url='https://httpbin.org/post',
        method='POST',
        payload=str(form_data).encode(),
    )

    await crawler.run([request])


Review comment (suggestion):
Depending on your content type, you might want to use `urlencode.urlencode` or some library that will let you encode it as a multipart form.

Code after:
    [CurlImpersonateHttpClient, HttpxHttpClient],
    ids=['curl', 'httpx'],
)
async def test_sending_payload(http_client_class: type[BaseHttpClient]) -> None:
    http_client = http_client_class()
    crawler = HttpCrawler(http_client=http_client)

    # Payload, e.g. data from a form submission.
    payload = {
        'custname': 'John Doe',
        'custtel': '1234567890',
        'custemail': 'johndoe@example.com',
        'size': 'large',
        'topping': '["bacon", "cheese", "mushroom"]',
        'delivery': '13:00',
        'comments': 'Please ring the doorbell upon arrival.',
    }

    responses = []

    @crawler.router.default_handler
    async def request_handler(context: HttpCrawlingContext) -> None:
        response = json.loads(context.http_response.read())
        # The httpbin.org/post endpoint returns the provided payload in the response.
        responses.append(response)

    request = Request.from_url(
        url='https://httpbin.org/post',
        method='POST',
        payload=urlencode(payload).encode(),
    )

    await crawler.run([request])

    # The request handler should be called once.
    assert len(responses) == 1

    # The reconstructed payload data should match the original payload. We have to flatten the values, because
    # parse_qs returns a list of values for each key.
    response_data = {
        k: v[0] if len(v) == 1 else v for k, v in parse_qs(responses[0]['data'].strip("b'").strip("'")).items()
    }

    assert response_data == payload
Repository: apify/crawlee-python
File: src/crawlee/storages/_key_value_store.py (Python)

Code before:
        value: Any,
        content_type: str | None = None,
    ) -> None:
        """Set a value in the KVS.

        Args:
            key: Key of the record to set.
            value: Value to set. If `None`, the record is deleted.
            content_type: Content type of the record.
        """
        if value is None:
            return await self._resource_client.delete_record(key)

        return await self._resource_client.set_record(key, value, content_type)

    def get_public_url(self, key: str) -> str:
        """Get the public URL for the given key.

        Args:
            key: Key of the record for which URL is required
        Returns:
            The public URL for the given key.
        """
        name = self.name or self._configuration.default_key_value_store_id
        storage_dir = self._configuration.storage_dir
        return f'file://{storage_dir}/storage/key_value_stores/{name}/{key}'


Review comment (suggestion):
Issue #514 mentions that we should put the actual logic for determining the public url into the `KeyValueStoreClient` class (file `src/crawlee/memory_storage_client/_key_value_store_client.py`). And call the method from here. Can you please do that?

This will allow the Apify SDK to replace the functionality in a cleaner fashion.

Code after:
        value: Any,
        content_type: str | None = None,
    ) -> None:
        """Set a value in the KVS.

        Args:
            key: Key of the record to set.
            value: Value to set. If `None`, the record is deleted.
            content_type: Content type of the record.
        """
        if value is None:
            return await self._resource_client.delete_record(key)

        return await self._resource_client.set_record(key, value, content_type)

    async def get_public_url(self, key: str) -> str:
        """Get the public URL for the given key.

        Args:
            key: Key of the record for which URL is required.

        Returns:
            The public URL for the given key.
        """
        return await self._resource_client.get_public_url(key)
Repository: apify/crawlee-python
File: src/crawlee/storages/_key_value_store.py (Python)

Code before:

    async def set_value(
        self,
        key: str,
        value: Any,
        content_type: str | None = None,
    ) -> None:
        """Set a value in the KVS.

        Args:
            key: Key of the record to set.
            value: Value to set. If `None`, the record is deleted.
            content_type: Content type of the record.
        """
        if value is None:
            return await self._resource_client.delete_record(key)

        return await self._resource_client.set_record(key, value, content_type)

    def get_public_url(self, key: str) -> str:
        """Get the public URL for the given key.

        Args:
            key: Key of the record for which URL is required
        Returns:
            The public URL for the given key.
        """
        name = self.name or self._configuration.default_key_value_store_id
        storage_dir = self._configuration.storage_dir
        return f'file://{storage_dir}/storage/key_value_stores/{name}/{key}'


Review comment (suggestion):
```suggestion
        Args:
            key: Key of the record for which URL is required

        Returns:
            The public URL for the given key.
```

Code after:

    async def set_value(
        self,
        key: str,
        value: Any,
        content_type: str | None = None,
    ) -> None:
        """Set a value in the KVS.

        Args:
            key: Key of the record to set.
            value: Value to set. If `None`, the record is deleted.
            content_type: Content type of the record.
        """
        if value is None:
            return await self._resource_client.delete_record(key)

        return await self._resource_client.set_record(key, value, content_type)

    async def get_public_url(self, key: str) -> str:
        """Get the public URL for the given key.

        Args:
            key: Key of the record for which URL is required.

        Returns:
            The public URL for the given key.
        """
        return await self._resource_client.get_public_url(key)
Repository: apify/crawlee-python
File: src/crawlee/base_storage_client/_base_key_value_store_client.py (Python)

Code before:
    @abstractmethod
    async def set_record(self, key: str, value: Any, content_type: str | None = None) -> None:
        """Set a value to the given record in the key-value store.

        Args:
            key: The key of the record to save the value to
            value: The value to save into the record
            content_type: The content type of the saved value
        """

    @abstractmethod
    async def delete_record(self, key: str) -> None:
        """Delete the specified record from the key-value store.

        Args:
            key: The key of the record which to delete
        """

    @abstractmethod
    def get_public_url(self, key: str, name: str | None, configuration: Configuration) -> str:
        """Get the public URL for the given key.

        Args:
            name: Identifier for the key value store
            key: Key of the record for which URL is required
            configuration: Configuration of the key value store

        Returns:
            The public URL for the given key.
        """


Review comment (suggestion):
This is also not necessary - you can already access this in memory storage clients and other implementations won't need it.

Code after:
    async def set_record(self, key: str, value: Any, content_type: str | None = None) -> None:
        """Set a value to the given record in the key-value store.

        Args:
            key: The key of the record to save the value to
            value: The value to save into the record
            content_type: The content type of the saved value
        """

    @abstractmethod
    async def delete_record(self, key: str) -> None:
        """Delete the specified record from the key-value store.

        Args:
            key: The key of the record which to delete
        """

    @abstractmethod
    async def get_public_url(self, key: str) -> str:
        """Get the public URL for the given key.

        Args:
            key: Key of the record for which URL is required.

        Returns:
            The public URL for the given key.

        Raises:
            ValueError: If the key does not exist.
        """
Repository: apify/crawlee-python
File: tests/unit/storages/test_key_value_store.py (Python)

Code before:


async def test_for_each_key(key_value_store: KeyValueStore) -> None:
    keys = [item.key async for item in key_value_store.iterate_keys()]
    assert len(keys) == 0

    for i in range(2001):
        await key_value_store.set_value(str(i).zfill(4), i)
    index = 0
    async for item in key_value_store.iterate_keys():
        assert item.key == str(index).zfill(4)
        index += 1
    assert index == 2001


async def test_static_get_set_value(key_value_store: KeyValueStore) -> None:
    await key_value_store.set_value('test-static', 'static')
    value = await key_value_store.get_value('test-static')
    assert value == 'static'


async def test_static_get_public_url(key_value_store: KeyValueStore) -> None:
    await key_value_store.set_value('test-static', 'static')
    public_url = key_value_store.get_public_url('test-static')

    path = unquote(urlparse(public_url).path)
    assert Path(path).read_text() == 'static'


Review comment (refactor):
```suggestion
    path = unquote(urlparse(public_url).path)  # extract path from a 'file:///' URI
```

Code after:


async def test_for_each_key(key_value_store: KeyValueStore) -> None:
    keys = [item.key async for item in key_value_store.iterate_keys()]
    assert len(keys) == 0

    for i in range(2001):
        await key_value_store.set_value(str(i).zfill(4), i)
    index = 0
    async for item in key_value_store.iterate_keys():
        assert item.key == str(index).zfill(4)
        index += 1
    assert index == 2001


async def test_static_get_set_value(key_value_store: KeyValueStore) -> None:
    await key_value_store.set_value('test-static', 'static')
    value = await key_value_store.get_value('test-static')
    assert value == 'static'


async def test_get_public_url_raises_for_non_existing_key(key_value_store: KeyValueStore) -> None:
    with pytest.raises(ValueError, match='was not found'):
        await key_value_store.get_public_url('i-do-not-exist')


async def test_get_public_url(key_value_store: KeyValueStore) -> None:
    await key_value_store.set_value('test-static', 'static')
    public_url = await key_value_store.get_public_url('test-static')

    url = urlparse(public_url)
    path = url.netloc if url.netloc else url.path

    with open(path) as f:  # noqa: ASYNC230
        content = await asyncio.to_thread(f.read)
        assert content == 'static'
Repository: apify/crawlee-python
File: tests/unit/storages/test_key_value_store.py (Python)

Code before:
    assert len(keys) == 0

    for i in range(2001):
        await key_value_store.set_value(str(i).zfill(4), i)
    index = 0
    async for item in key_value_store.iterate_keys():
        assert item.key == str(index).zfill(4)
        index += 1
    assert index == 2001


async def test_static_get_set_value(key_value_store: KeyValueStore) -> None:
    await key_value_store.set_value('test-static', 'static')
    value = await key_value_store.get_value('test-static')
    assert value == 'static'


async def test_static_get_public_url(key_value_store: KeyValueStore) -> None:
    await key_value_store.set_value('test-static', 'static')
    public_url = await key_value_store.get_public_url('test-static')

    url = urlparse(public_url)
    path = url.netloc if url.netloc else url.path

    f = await asyncio.to_thread(open, path, mode='r')
    assert f.read() == 'static'


Review comment (suggestion):
```suggestion
    with open(path, mode='r') as f:
        content = await asyncio.to_thread(f.read)
        assert content == 'static'
```

Code after:
    assert len(keys) == 0

    for i in range(2001):
        await key_value_store.set_value(str(i).zfill(4), i)
    index = 0
    async for item in key_value_store.iterate_keys():
        assert item.key == str(index).zfill(4)
        index += 1
    assert index == 2001


async def test_static_get_set_value(key_value_store: KeyValueStore) -> None:
    await key_value_store.set_value('test-static', 'static')
    value = await key_value_store.get_value('test-static')
    assert value == 'static'


async def test_get_public_url_raises_for_non_existing_key(key_value_store: KeyValueStore) -> None:
    with pytest.raises(ValueError, match='was not found'):
        await key_value_store.get_public_url('i-do-not-exist')


async def test_get_public_url(key_value_store: KeyValueStore) -> None:
    await key_value_store.set_value('test-static', 'static')
    public_url = await key_value_store.get_public_url('test-static')

    url = urlparse(public_url)
    path = url.netloc if url.netloc else url.path

    with open(path) as f:  # noqa: ASYNC230
        content = await asyncio.to_thread(f.read)
        assert content == 'static'
Repository: apify/crawlee-python
File: src/crawlee/base_storage_client/_base_key_value_store_client.py (Python)

Code before:
        """

    @abstractmethod
    async def set_record(self, key: str, value: Any, content_type: str | None = None) -> None:
        """Set a value to the given record in the key-value store.

        Args:
            key: The key of the record to save the value to
            value: The value to save into the record
            content_type: The content type of the saved value
        """

    @abstractmethod
    async def delete_record(self, key: str) -> None:
        """Delete the specified record from the key-value store.

        Args:
            key: The key of the record which to delete
        """

    @abstractmethod
    async def get_public_url(self, key: str) -> str:
        """Get the public URL for the given key.

        Args:
            key: Key of the record for which URL is required

        Returns:
            The public URL for the given key.
        """


Review comment (suggestion):
```suggestion
            key: Key of the record for which URL is required.
```

Code after:
        """

    @abstractmethod
    async def set_record(self, key: str, value: Any, content_type: str | None = None) -> None:
        """Set a value to the given record in the key-value store.

        Args:
            key: The key of the record to save the value to
            value: The value to save into the record
            content_type: The content type of the saved value
        """

    @abstractmethod
    async def delete_record(self, key: str) -> None:
        """Delete the specified record from the key-value store.

        Args:
            key: The key of the record which to delete
        """

    @abstractmethod
    async def get_public_url(self, key: str) -> str:
        """Get the public URL for the given key.

        Args:
            key: Key of the record for which URL is required.

        Returns:
            The public URL for the given key.

        Raises:
            ValueError: If the key does not exist.
        """
Repository: apify/crawlee-python
File: src/crawlee/storages/_key_value_store.py (Python)

Code before:
                break
            exclusive_start_key = list_keys.next_exclusive_start_key

    async def set_value(
        self,
        key: str,
        value: Any,
        content_type: str | None = None,
    ) -> None:
        """Set a value in the KVS.

        Args:
            key: Key of the record to set.
            value: Value to set. If `None`, the record is deleted.
            content_type: Content type of the record.
        """
        if value is None:
            return await self._resource_client.delete_record(key)

        return await self._resource_client.set_record(key, value, content_type)

    async def get_public_url(self, key: str) -> str:
        """Get the public URL for the given key.

        Args:
            key: Key of the record for which URL is required

        Returns:
            The public URL for the given key.
        """
        return await self._resource_client.get_public_url(key)


Review comment (suggestion):
```suggestion
            key: Key of the record for which URL is required.
```

Code after:
                break
            exclusive_start_key = list_keys.next_exclusive_start_key

    async def set_value(
        self,
        key: str,
        value: Any,
        content_type: str | None = None,
    ) -> None:
        """Set a value in the KVS.

        Args:
            key: Key of the record to set.
            value: Value to set. If `None`, the record is deleted.
            content_type: Content type of the record.
        """
        if value is None:
            return await self._resource_client.delete_record(key)

        return await self._resource_client.set_record(key, value, content_type)

    async def get_public_url(self, key: str) -> str:
        """Get the public URL for the given key.

        Args:
            key: Key of the record for which URL is required.

        Returns:
            The public URL for the given key.
        """
        return await self._resource_client.get_public_url(key)