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.

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

Add apply_apify_settings to Scrapy subpackage #178

Merged
merged 4 commits into from
Jan 23, 2024
Merged

Add apply_apify_settings to Scrapy subpackage #178

merged 4 commits into from
Jan 23, 2024

Conversation

vdusek
Copy link
Contributor

@vdusek vdusek commented Jan 22, 2024 •
edited

Issue

Description

  • This PR moves just a single function from a template to SDK - apply_apify_settings.
  • The rest of the suggested changes are related to the logging. I was not able to make them work at first try. So for now let's solve it just for the settings-related helper function, and the rest will be done later.

Testing

  • The changes were tested both locally and on the platform.
  • Unit tests are ready as well.

@vdusek vdusek changed the title Add apply_apify_settings Add apply_apify_settings to Scrapy subpackage Jan 22, 2024
@vdusek vdusek added t-tooling Issues with this label are in the ownership of the tooling team. adhoc Ad-hoc unplanned task added during the sprint. enhancement New feature or request. labels Jan 22, 2024
@vdusek vdusek added this to the 81st sprint - Tooling team milestone Jan 22, 2024
@vdusek
Copy link
Contributor Author

vdusek commented Jan 22, 2024

@honzajavorek FYI; also if you would have any feedback on this, I'll be glad. Thanks 🙂.

@vdusek vdusek requested a review from janbuchar January 22, 2024 12:36
@vdusek vdusek mentioned this pull request Jan 22, 2024
Merged
honzajavorek
honzajavorek suggested changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@honzajavorek honzajavorek left a comment •
edited

Choose a reason for hiding this comment

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

The implementation looks good, awesome! I've found a few places in the tests where I commented, but they're nothing serious except for the @pytest.mark.only() leftover, which was the decisive factor in why I'm requesting changes.

tests/unit/scrapy/utils/test_apply_apify_settings.py Outdated Show resolved Hide resolved
tests/unit/scrapy/utils/test_apply_apify_settings.py Outdated Show resolved Hide resolved
tests/unit/scrapy/utils/test_apply_apify_settings.py Outdated Show resolved Hide resolved
janbuchar
janbuchar approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@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.

Looks good to me.

tests/unit/scrapy/utils/test_apply_apify_settings.py Show resolved Hide resolved
@vdusek
Copy link
Contributor Author

vdusek commented Jan 23, 2024

Thank you both guys @honzajavorek, @janbuchar for your opinions. Merging it.

@vdusek vdusek merged commit 72a37f1 into master Jan 23, 2024
23 checks passed
@vdusek vdusek deleted the improve-scrapy-integration branch January 23, 2024 12:40
@vdusek vdusek mentioned this pull request Jan 23, 2024
Open
@vdusek vdusek self-assigned this Jan 23, 2024
@honzajavorek
Copy link
Contributor

Thanks for shipping it! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fnesveda fnesveda fnesveda left review comments

@janbuchar janbuchar janbuchar approved these changes

@honzajavorek honzajavorek Awaiting requested review from honzajavorek

Assignees

@vdusek vdusek

Labels
adhoc Ad-hoc unplanned task added during the sprint. enhancement New feature or request. t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Milestone
81st sprint - Tooling team
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vdusek @honzajavorek @janbuchar @fnesveda