Skip to content

feat: Add BeautifulSoupParser type alias #674

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 11, 2024
Merged

feat: Add BeautifulSoupParser type alias #674

merged 2 commits into from
Nov 11, 2024

Conversation

@Pijukatel
Copy link
Collaborator

@Pijukatel Pijukatel commented Nov 10, 2024 •
edited
Loading

Description

To avoid repeating same Literal definitions.

Checklist

  • CI passed

@Pijukatel Pijukatel requested a review from vdusek November 10, 2024 12:15
@github-actions github-actions bot added this to the 102nd sprint - Tooling team milestone Nov 10, 2024
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Nov 10, 2024
@Pijukatel
Copy link
Collaborator Author

Pijukatel commented Nov 10, 2024

Use case:
apify/actor-beautifulsoup-scraper@master...refactor_to_use_crawlee#diff-ac8f5a31e674c80868c8fca98ec6a0b77707c58f115c23dfaf31476a2b95b793R34

github-actions[bot]
github-actions bot reviewed Nov 10, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Pull Request Tookit has failed!

Pull request is neither linked to an issue or epic nor labeled as adhoc!

janbuchar
janbuchar reviewed Nov 10, 2024
View reviewed changes
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.

Sure, this makes sense. Could you consider reexporting the new type so that you can import it without accessing private subpackages?

@B4nan B4nan changed the title Add BeautifoulSoupParser for typing. Add BeautifulSoupParser for typing. Nov 10, 2024
@Pijukatel
Copy link
Collaborator Author

Pijukatel commented Nov 10, 2024 •
edited
Loading

Sure, this makes sense. Could you consider reexporting the new type so that you can import it without accessing private subpackages?

Sure. Just a tiny question. What is the established practice here with _all_? I know it is for star imports, but do you put all stuff inside or just some selected top level stuff?

@janbuchar
Copy link
Collaborator

janbuchar commented Nov 10, 2024

You should include only names that are part of the "public interface" of the package in __all__.

@Pijukatel
Add BeautifoulSoupParser for typing.
4c28b49 
To avoid repeating same Literal definitions.
github-actions[bot]
github-actions bot reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Pull Request Tookit has failed!

Pull request is neither linked to an issue or epic nor labeled as adhoc!

@Pijukatel Pijukatel changed the title Add BeautifulSoupParser for typing. refactor: Add BeautifulSoupParser for typing. Nov 11, 2024
@Pijukatel Pijukatel added the adhoc Ad-hoc unplanned task added during the sprint. label Nov 11, 2024
github-actions[bot]
github-actions bot reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Pull Request Tookit has failed!

If issue is not linked to the pull request then estimate the pull request!

@vdusek
Copy link
Collaborator

vdusek commented Nov 11, 2024

I will also add that the __all__ field is there primarily because some Python tooling uses it to discover the package's public interface.

@vdusek vdusek changed the title refactor: Add BeautifulSoupParser for typing. refactor: Add BeautifulSoupParser for typing Nov 11, 2024
@janbuchar
Copy link
Collaborator

janbuchar commented Nov 11, 2024

@Pijukatel And I will add this - it's a nice read: https://mkdocstrings.github.io/griffe/guide/users/recommendations/public-apis/

@janbuchar janbuchar self-requested a review November 11, 2024 11:29
janbuchar
janbuchar approved these changes Nov 11, 2024
View reviewed changes
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 approved these changes Nov 11, 2024
View reviewed changes
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.

LGTM

@Pijukatel Pijukatel changed the title refactor: Add BeautifulSoupParser for typing feat: Add BeautifulSoupParser for typing Nov 11, 2024
@vdusek vdusek changed the title feat: Add BeautifulSoupParser for typing feat: Add BeautifulSoupParser type alias Nov 11, 2024
@Pijukatel Pijukatel merged commit b2cf88f into master Nov 11, 2024
25 checks passed
@Pijukatel Pijukatel deleted the define-bs-parser branch November 11, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@janbuchar janbuchar janbuchar approved these changes

@vdusek vdusek vdusek approved these changes

Assignees

@Pijukatel Pijukatel

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Milestone

102nd sprint - Tooling team

Development

Successfully merging this pull request may close these issues.

4 participants

@Pijukatel @janbuchar @vdusek