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

Code before:
        nonlocal text
        for page_element in page_elements:
            if isinstance(page_element, (Tag, NavigableString)):
                if isinstance(page_element, NavigableString):
                    compr: str
                    if isinstance(page_element.parent, Tag) and page_element.parent.name.lower() == 'pre':
                        compr = page_element.get_text()
                    else:
                        # Compress white spaces outside of pre block
                        compr = re.sub(r'\s+', ' ', page_element.get_text())
                    # If text is empty or ends with a whitespace, don't add the leading whitespace or new line
                    if (compr.startswith((' ', '\n'))) and re.search(r'(^|\s)$', text):
                        compr = compr[1:]
                    text += compr
                elif page_element.name.lower() in SKIP_TAGS or isinstance(page_element, int):
                    # Skip comments and special elements
                    pass
                elif page_element.name.lower() == 'br':
                    text += '\n'
                elif page_element.name.lower() == 'td':
                    _page_element_to_text(page_element.children)
                    text += '\t'
                else:
                    # Block elements must be surrounded by newlines(unless beginning of text)
                    is_block_tag = page_element.name.lower() in BLOCK_TAGS
                    if is_block_tag and not re.search(r'(^|\n)$', text):
                        text += '\n'
                    _page_element_to_text(page_element.children)
                    if is_block_tag and not text.endswith('\n'):
                        text += '\n'

    _page_element_to_text(soup.children)
    return text.strip()


Review comment (suggestion):
Is it possible to use regex with pre-compile `re.compile`? I think this would be useful if the function will be used frequently during the crawler process.

Code after:
# This file contains shared constants used by different implementations of html_to_text function.
from __future__ import annotations

import re

# Tags based on Javascript implementation of htmlToText from:
# https://github.com/apify/crawlee/blob/master/packages/utils/src/internals/cheerio.ts#L11
# Originally added here: https://github.com/apify/apify-ts/commit/4c0e5e3e7377536a449bb7b205132348ad3b0fe9
SKIP_TAGS = {'script', 'style', 'canvas', 'svg', 'noscript', 'title'}
BLOCK_TAGS = {
    'p',
    'h1',
    'h2',
    'h3',
    'h4',
    'h5',
    'h6',
    'ol',
    'ul',
    'li',
    'pre',
    'address',
    'blockquote',
    'dl',
    'div',
    'fieldset',
    'form',
    'table',
    'tr',
    'select',
    'option',
}

_EMPTY_OR_ENDS_WITH_ANY_WHITE_SPACE = re.compile(r'(^|\s)$')
_EMPTY_OR_ENDS_WITH_NEW_LINE = re.compile(r'(^|\n)$')
_ANY_CONSECUTIVE_WHITE_SPACES = re.compile(r'\s+')
Repository: apify/crawlee-python
File: src/crawlee/_utils/html_to_text.py (Python)

Code before:
from __future__ import annotations

import re
from typing import TYPE_CHECKING

from bs4 import BeautifulSoup, NavigableString, PageElement, Tag

if TYPE_CHECKING:
    from collections.abc import Iterable

SKIP_TAGS = {'script', 'style', 'canvas', 'svg', 'noscript', 'title'}
BLOCK_TAGS = {
    'p',
    'h1',
    'h2',
    'h3',
    'h4',
    'h5',
    'h6',
    'ol',
    'ul',
    'li',
    'pre',
    'address',
    'blockquote',
    'dl',
    'div',

Review comment (suggestion):
I would prefer moving this to e.g. `crawlee/beautifulsoup_crawler/_utils.py` to avoid the need for try-except imports here.

Code after:
# This file contains shared constants used by different implementations of html_to_text function.
from __future__ import annotations

import re

# Tags based on Javascript implementation of htmlToText from:
# https://github.com/apify/crawlee/blob/master/packages/utils/src/internals/cheerio.ts#L11
# Originally added here: https://github.com/apify/apify-ts/commit/4c0e5e3e7377536a449bb7b205132348ad3b0fe9
SKIP_TAGS = {'script', 'style', 'canvas', 'svg', 'noscript', 'title'}
BLOCK_TAGS = {
    'p',
    'h1',
    'h2',
    'h3',
    'h4',
    'h5',
    'h6',
    'ol',
    'ul',
    'li',
    'pre',
    'address',
    'blockquote',
    'dl',
    'div',
    'fieldset',
    'form',
Repository: apify/crawlee-python
File: src/crawlee/beautifulsoup_crawler/_beautifulsoup_crawling_context.py (Python)

Code before:

from crawlee._utils.docs import docs_group
from crawlee.abstract_http_crawler._http_crawling_context import ParsedHttpCrawlingContext
from crawlee.beautifulsoup_crawler._utils import html_to_text


@dataclass(frozen=True)
@docs_group('Data structures')
class BeautifulSoupCrawlingContext(ParsedHttpCrawlingContext[BeautifulSoup]):
    """The crawling context used by the `BeautifulSoupCrawler`.

    It provides access to key objects as well as utility functions for handling crawling tasks.
    """

    @property
    def soup(self) -> BeautifulSoup:
        """Convenience alias."""
        return self.parsed_content

    @classmethod
    def from_parsed_http_crawling_context(cls, context: ParsedHttpCrawlingContext[BeautifulSoup]) -> Self:
        """Convenience constructor that creates new context from existing `ParsedHttpCrawlingContext[BeautifulSoup]`."""
        return cls(**{field.name: getattr(context, field.name) for field in fields(context)})

    def html_to_text(self) -> str:
        """Converts parsed_content to newline-separated plain text without tags."""
        return html_to_text(self.parsed_content)


Review comment (suggestion):
```suggestion
        """Convert the parsed HTML content to newline-separated plain text without tags."""
```

Code after:

from crawlee._utils.docs import docs_group
from crawlee.abstract_http_crawler._http_crawling_context import ParsedHttpCrawlingContext
from crawlee.beautifulsoup_crawler._utils import html_to_text


@dataclass(frozen=True)
@docs_group('Data structures')
class BeautifulSoupCrawlingContext(ParsedHttpCrawlingContext[BeautifulSoup]):
    """The crawling context used by the `BeautifulSoupCrawler`.

    It provides access to key objects as well as utility functions for handling crawling tasks.
    """

    @property
    def soup(self) -> BeautifulSoup:
        """Convenience alias."""
        return self.parsed_content

    @classmethod
    def from_parsed_http_crawling_context(cls, context: ParsedHttpCrawlingContext[BeautifulSoup]) -> Self:
        """Convenience constructor that creates new context from existing `ParsedHttpCrawlingContext[BeautifulSoup]`."""
        return cls(**{field.name: getattr(context, field.name) for field in fields(context)})

    def html_to_text(self) -> str:
        """Convert the parsed HTML content to newline-separated plain text without tags."""
        return html_to_text(self.parsed_content)
Repository: apify/crawlee-python
File: src/crawlee/parsel_crawler/_parsel_crawling_context.py (Python)

Code before:

from crawlee._utils.docs import docs_group
from crawlee.abstract_http_crawler._http_crawling_context import ParsedHttpCrawlingContext
from crawlee.parsel_crawler._utils import html_to_text


@dataclass(frozen=True)
@docs_group('Data structures')
class ParselCrawlingContext(ParsedHttpCrawlingContext[Selector]):
    """The crawling context used by the `ParselCrawler`.

    It provides access to key objects as well as utility functions for handling crawling tasks.
    """

    @property
    def selector(self) -> Selector:
        """Convenience alias."""
        return self.parsed_content

    @classmethod
    def from_parsed_http_crawling_context(cls, context: ParsedHttpCrawlingContext[Selector]) -> Self:
        """Convenience constructor that creates new context from existing `ParsedHttpCrawlingContext[BeautifulSoup]`."""
        return cls(**{field.name: getattr(context, field.name) for field in fields(context)})

    def html_to_text(self) -> str:
        """Converts parsed_content to newline-separated plain text without tags."""
        return html_to_text(self.parsed_content)


Review comment (suggestion):
```suggestion
        """Convert the parsed HTML content to newline-separated plain text without tags."""
```

Code after:

from crawlee._utils.docs import docs_group
from crawlee.abstract_http_crawler._http_crawling_context import ParsedHttpCrawlingContext
from crawlee.parsel_crawler._utils import html_to_text


@dataclass(frozen=True)
@docs_group('Data structures')
class ParselCrawlingContext(ParsedHttpCrawlingContext[Selector]):
    """The crawling context used by the `ParselCrawler`.

    It provides access to key objects as well as utility functions for handling crawling tasks.
    """

    @property
    def selector(self) -> Selector:
        """Convenience alias."""
        return self.parsed_content

    @classmethod
    def from_parsed_http_crawling_context(cls, context: ParsedHttpCrawlingContext[Selector]) -> Self:
        """Convenience constructor that creates new context from existing `ParsedHttpCrawlingContext[BeautifulSoup]`."""
        return cls(**{field.name: getattr(context, field.name) for field in fields(context)})

    def html_to_text(self) -> str:
        """Convert the parsed HTML content to newline-separated plain text without tags."""
        return html_to_text(self.parsed_content)
Repository: apify/crawlee-python
File: src/crawlee/_utils/html_to_text.py (Python)

Code before:
# This file contains shared constants used by different implementations of html_to_text function.
from __future__ import annotations

import re

# Tags based on Javascript implementation of text_to_html from:
# https://github.com/apify/crawlee/blob/master/packages/utils/src/internals/cheerio.ts#L11
# Originally added here: https://github.com/apify/apify-ts/commit/4c0e5e3e7377536a449bb7b205132348ad3b0fe9
SKIP_TAGS = {'script', 'style', 'canvas', 'svg', 'noscript', 'title'}
BLOCK_TAGS = {
    'p',
    'h1',
    'h2',
    'h3',
    'h4',
    'h5',
    'h6',
    'ol',
    'ul',
    'li',
    'pre',
    'address',
    'blockquote',
    'dl',
    'div',
    'fieldset',
    'form',
    'table',
    'tr',
    'select',
    'option',

Review comment (suggestion):
```suggestion
# Tags based on Javascript implementation of htmlToText from:
```

Code after:
# This file contains shared constants used by different implementations of html_to_text function.
from __future__ import annotations

import re

# Tags based on Javascript implementation of htmlToText from:
# https://github.com/apify/crawlee/blob/master/packages/utils/src/internals/cheerio.ts#L11
# Originally added here: https://github.com/apify/apify-ts/commit/4c0e5e3e7377536a449bb7b205132348ad3b0fe9
SKIP_TAGS = {'script', 'style', 'canvas', 'svg', 'noscript', 'title'}
BLOCK_TAGS = {
    'p',
    'h1',
    'h2',
    'h3',
    'h4',
    'h5',
    'h6',
    'ol',
    'ul',
    'li',
    'pre',
    'address',
    'blockquote',
    'dl',
    'div',
    'fieldset',
    'form',
    'table',
    'tr',
    'select',
    'option',
Repository: apify/crawlee-python
File: src/crawlee/events/_types.py (Python)

Code before:
from __future__ import annotations

from collections.abc import Callable, Coroutine
from enum import Enum
from typing import Annotated, Any, TypeVar, Union

from pydantic import BaseModel, ConfigDict, Field

from crawlee._utils.docs import docs_group
from crawlee._utils.system import CpuInfo, MemoryUsageInfo


class Event(str, Enum):
    """Names of all possible events that can be emitted using an EventManager."""

    # Core events
    PERSIST_STATE = 'persistState'
    SYSTEM_INFO = 'systemInfo'
    MIGRATING = 'migrating'
    ABORTING = 'aborting'
    EXIT = 'exit'

    # Session pool events
    SESSION_RETIRED = 'sessionRetired'

    # Browser pool events
    BROWSER_LAUNCHED = 'browserLaunched'
    BROWSER_RETIRED = 'browserRetired'
    BROWSER_CLOSED = 'browserClosed'
    PAGE_CREATED = 'pageCreated'
    PAGE_CLOSED = 'pageClosed'


@docs_group('Event payloads')
class EventPersistStateData(BaseModel):
    """Data for the persist state event."""

    model_config = ConfigDict(populate_by_name=True)


Review comment (suggestion):
```suggestion
    """Names of all possible events that can be emitted using an `EventManager`."""
```

Code after:
from __future__ import annotations

from collections.abc import Callable, Coroutine
from enum import Enum
from typing import Annotated, Any, TypeVar, Union

from pydantic import BaseModel, ConfigDict, Field

from crawlee._utils.docs import docs_group
from crawlee._utils.system import CpuInfo, MemoryUsageInfo


class Event(str, Enum):
    """Names of all possible events that can be emitted using an `EventManager`."""

    # Core events
    PERSIST_STATE = 'persistState'
    SYSTEM_INFO = 'systemInfo'
    MIGRATING = 'migrating'
    ABORTING = 'aborting'
    EXIT = 'exit'

    # Session pool events
    SESSION_RETIRED = 'sessionRetired'

    # Browser pool events
    BROWSER_LAUNCHED = 'browserLaunched'
    BROWSER_RETIRED = 'browserRetired'
    BROWSER_CLOSED = 'browserClosed'
    PAGE_CREATED = 'pageCreated'
    PAGE_CLOSED = 'pageClosed'


@docs_group('Event payloads')
class EventPersistStateData(BaseModel):
    """Data for the persist state event."""

    model_config = ConfigDict(populate_by_name=True)
Repository: apify/crawlee-python
File: tests/unit/basic_crawler/test_basic_crawler.py (Python)

Code before:
    assert stats.requests_finished == 2


async def test_crawler_multiple_stops_in_parallel(httpbin: URL) -> None:
    """Test that no new requests are handled after crawler.stop() is called, but ongoing requests can still finish."""
    start_urls = [
        str(httpbin / '1'),
        str(httpbin / '2'),
        str(httpbin / '3'),
    ]
    processed_urls = []

    # Set max_concurrency to 2 to ensure two urls are being visited in parallel.
    crawler = BasicCrawler(concurrency_settings=ConcurrencySettings(max_concurrency=2))

    sleep_time_generator = iter([0, 0.1])

    @crawler.router.default_handler
    async def handler(context: BasicCrawlingContext) -> None:
        processed_urls.append(context.request.url)

        # This sleep ensures that first request is processed quickly and triggers stop() almost immediately.
        # Second request will have some sleep time to make sure it is still being processed after crawler.stop() was
        # called from the first request and so the crawler is already shutting down.
        await asyncio.sleep(next(sleep_time_generator))
        crawler.stop(reason=f'Stop called on  {context.request.url}')

    stats = await crawler.run(start_urls)

    # Verify that only 2 out of the 3 provided URLs were made
    assert len(processed_urls) == 2
    assert stats.requests_total == 2
    assert stats.requests_finished == 2


Review comment (suggestion):
```suggestion
        crawler.stop(reason=f'Stop called on {context.request.url}')
```

Code after:
    assert stats.requests_finished == 2


async def test_crawler_multiple_stops_in_parallel(httpbin: URL) -> None:
    """Test that no new requests are handled after crawler.stop() is called, but ongoing requests can still finish."""
    start_urls = [
        str(httpbin / '1'),
        str(httpbin / '2'),
        str(httpbin / '3'),
    ]
    processed_urls = []

    # Set max_concurrency to 2 to ensure two urls are being visited in parallel.
    crawler = BasicCrawler(concurrency_settings=ConcurrencySettings(max_concurrency=2))

    sleep_time_generator = iter([0, 0.1])

    @crawler.router.default_handler
    async def handler(context: BasicCrawlingContext) -> None:
        processed_urls.append(context.request.url)

        # This sleep ensures that first request is processed quickly and triggers stop() almost immediately.
        # Second request will have some sleep time to make sure it is still being processed after crawler.stop() was
        # called from the first request and so the crawler is already shutting down.
        await asyncio.sleep(next(sleep_time_generator))
        crawler.stop(reason=f'Stop called on {context.request.url}')

    stats = await crawler.run(start_urls)

    # Verify that only 2 out of the 3 provided URLs were made
    assert len(processed_urls) == 2
    assert stats.requests_total == 2
    assert stats.requests_finished == 2
Repository: apify/crawlee-python
File: src/crawlee/http_clients/_base.py (Python)

Code before:
        Raises:
            ProxyError: Raised if a proxy-related error occurs.
            HttpStatusError: Raised if the response status code indicates an error.

        Returns:
            The HTTP response received from the server.
        """

    def _raise_for_error_status_code(
        self,
        status_code: int,
        additional_http_error_status_codes: set[int],
        ignore_http_error_status_codes: set[int],
    ) -> None:
        """Raise an exception if the given status code is considered as an error."""
        exclude_error = status_code in ignore_http_error_status_codes
        include_error = status_code in additional_http_error_status_codes

        if include_error:
            raise HttpStatusCodeError('Error status code (user-configured) returned.', status_code)

        if is_status_code_client_error(status_code) and not exclude_error:
            raise HttpClientStatusCodeError('Client error status code returned', status_code)

        if is_status_code_server_error(status_code) and not exclude_error:
            raise HttpStatusCodeError('Error status code returned', status_code)

    @property
    def additional_blocked_status_codes(self) -> set[int]:
        return self._additional_http_error_status_codes

    @property
    def ignore_http_error_status_codes(self) -> set[int]:
        return self._ignore_http_error_status_codes


Review comment (suggestion):
Since the code changed, could you please update the local identifiers ? e.g. `exclude_error` => `is_ignored_status`, `include_error` => `is_explicit_error`.

Code after:
        Raises:
            ProxyError: Raised if a proxy-related error occurs.
            HttpStatusError: Raised if the response status code indicates an error.

        Returns:
            The HTTP response received from the server.
        """

    def _raise_for_error_status_code(
        self,
        status_code: int,
        additional_http_error_status_codes: set[int],
        ignore_http_error_status_codes: set[int],
    ) -> None:
        """Raise an exception if the given status code is considered as an error."""
        is_ignored_status = status_code in ignore_http_error_status_codes
        is_explicit_error = status_code in additional_http_error_status_codes

        if is_explicit_error:
            raise HttpStatusCodeError('Error status code (user-configured) returned.', status_code)

        if is_status_code_client_error(status_code) and not is_ignored_status:
            raise HttpClientStatusCodeError('Client error status code returned', status_code)

        if is_status_code_server_error(status_code) and not is_ignored_status:
            raise HttpStatusCodeError('Error status code returned', status_code)

    @property
    def additional_blocked_status_codes(self) -> set[int]:
        return self._additional_http_error_status_codes

    @property
    def ignore_http_error_status_codes(self) -> set[int]:
        return self._ignore_http_error_status_codes
Repository: apify/crawlee-python
File: src/crawlee/sessions/_session.py (Python)

Code before:
        """
        self._error_score += self._max_error_score
        self._usage_count += 1
        # Note: We emit an event here because of the Puppeteer in TS implementation.

    def is_blocked_status_code(
        self,
        *,
        status_code: int,
        additional_blocked_status_codes: set[int] | None = None,
        ignore_http_error_status_codes: set[int] | None = None,
    ) -> bool:
        """Evaluate whether a session should be retired based on the received HTTP status code.

        Args:
            status_code: The HTTP status code received from a server response.
            additional_blocked_status_codes: Optional additional status codes that should trigger session retirement.
            ignore_http_error_status_codes: Optional status codes to allow suppression of
            codes from `blocked_status_codes`.

        Returns:
            True if the session should be retired, False otherwise.
        """
        blocked_status_codes = self._blocked_status_codes | (additional_blocked_status_codes or set())

        blocked_status_codes = blocked_status_codes - (ignore_http_error_status_codes or set())

        return status_code in blocked_status_codes


Review comment (bug):
Not sure if I'm reading this right, but it seems to me that `ignore_http_error_status_codes` takes precedence over `additional_blocked_status_codes` here, but in `BaseHttpClient`, the `additional_http_error_status_codes` option takes precedence over `ignore_http_error_status_codes`.

This is an edge case for sure, but still, I'd like to keep things consistent.

Code after:
        """
        self._error_score += self._max_error_score
        self._usage_count += 1
        # Note: We emit an event here because of the Puppeteer in TS implementation.

    def is_blocked_status_code(
        self,
        *,
        status_code: int,
        additional_blocked_status_codes: set[int] | None = None,
        ignore_http_error_status_codes: set[int] | None = None,
    ) -> bool:
        """Evaluate whether a session should be retired based on the received HTTP status code.

        Args:
            status_code: The HTTP status code received from a server response.
            additional_blocked_status_codes: Optional additional status codes that should trigger session retirement.
            ignore_http_error_status_codes: Optional status codes to allow suppression of
            codes from `blocked_status_codes`.

        Returns:
            True if the session should be retired, False otherwise.
        """
        if additional_blocked_status_codes and status_code in additional_blocked_status_codes:
            return True

        return status_code in (self._blocked_status_codes - (ignore_http_error_status_codes or set()))
Repository: apify/crawlee-python
File: src/crawlee/crawlers/_playwright/_types.py (Python)

Code before:
from __future__ import annotations

from typing import Protocol


class BlockRequestsFunction(Protocol):
    """Protocol defining the interface for block_requests function."""

    async def __call__(
        self, url_patterns: list[str] | None = None, extra_url_patterns: list[str] | None = None
    ) -> None:
        """Blocks network requests matching specified URL patterns. Works only for Chromium browser.

        Args:
            url_patterns: List of URL patterns to block. If None, uses default patterns.
            extra_url_patterns: Additional URL patterns to append to the main patterns list.
        """
        ...


Review comment (suggestion):
```suggestion
@docs_group('Functions')
class BlockRequestsFunction(Protocol):
```

Code after:
from __future__ import annotations

from typing import Protocol

from crawlee._utils.docs import docs_group


@docs_group('Functions')
class BlockRequestsFunction(Protocol):
    """A function for blocking unwanted HTTP requests during page loads in PlaywrightCrawler.

    It simplifies the process of blocking specific HTTP requests during page navigation.
    The function allows blocking both default resource types (like images, fonts, stylesheets) and custom URL patterns.
    """

    async def __call__(
        self, url_patterns: list[str] | None = None, extra_url_patterns: list[str] | None = None
    ) -> None:
        """Call dunder method.

        Args:
            url_patterns: List of URL patterns to block. If None, uses default patterns.
            extra_url_patterns: Additional URL patterns to append to the main patterns list.
        """