-
Notifications
You must be signed in to change notification settings - Fork 224
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
Add a scraper for german indeed #136
Conversation
This is awesome, thanks so much for the contribution! But do you mind adding a test for your new demo file to https://github.com/PaulMcInnis/JobFunnel/blob/master/.github/workflows/ci.yml? When you add it, github Actions will automatically test your Germany scraper every time there is a push :). |
Thanks for the feedback. I have added a test run in the ci build, but the pipeline already fails due to existing issues. |
Thanks for adding it to the CI. Yes, we have issues in the CI. In fact me and @PaulMcInnis have been talking about this on #133. I thought I had fixed the issue with #134, but sadly it looks like it wasn't fixed completely :(. I'll try to look into it when I get time. |
-incoming_jobs_dict was the same value as jobs dictionary which is why _check_for_inter_scraper_validity was failing. Monster was comparing itself
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 36.12% 36.41% +0.28%
==========================================
Files 22 26 +4
Lines 1456 1494 +38
==========================================
+ Hits 526 544 +18
- Misses 930 950 +20
Continue to review full report at Codecov.
|
I think at least now we won't have job_id issues anymore. What's left is deciding what to do about the error code when we don't find any jobs. |
Cool, thank you for your changes! I think the status code should not be dependent on the number of results. One possibility to do this differently is by tracking for errors along the way and setting the status code after execution accordingly. However this is not related to the current feature, so I would do this in another pull request, if you agree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just some minor comments, but I think the improved keying will help us avoid issues in the future.
We should also rev up the major version as we are introducing a breaking change for users who wish to continue an existing search by changing the keying.
also once this goes in we should cut a new release as I think some of our recent issues are resolved by the current master. |
OK, going to merge this and cut a release with removed brotli encoding for all the other scrapers as well since it seems to be causing issues all around. Was a bit pre-emptive with 3.0.2 |
Add a scraper for german indeed
Description
Implemented a scraper implementation for the german indeed website.
The functionality includes a subset of the changes suggested in #132 by @Luckyz7 and the required changes were commented. A different locale name was used to align more closely with iso codes.
Context of change
Please add options that are relevant and mark any boxes that apply.
Type of change
Please mark any boxes that apply.
How Has This Been Tested?
Tested manually using a configuration in the demo directory.
Checklist:
Please mark any boxes that have been completed.