-
Notifications
You must be signed in to change notification settings - Fork 16
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
scrapy-spider-metadata support. #75
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #75 +/- ##
==========================================
- Coverage 95.58% 93.94% -1.64%
==========================================
Files 15 15
Lines 747 760 +13
==========================================
Hits 714 714
- Misses 33 46 +13
☔ View full report in Codecov by Sentry. |
I haven't added tests because the project doesn't currently test the command output either. It also needs a Scrapy project, which is possible (we generate Scrapy projects in tests in other libraries). |
# make sure it's serializable | ||
json.dumps(metadata_dict) | ||
except (TypeError, ValueError): | ||
continue |
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.
should we have a warning here?
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.
The command prints a JSON text which some other components read, we could print warnings to stderr but I don't think anything will read them (but I'm not sure).
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.
Still, it seems currently it's the only place where the error could be show; without it there is no indication of issue at all. But maybe there is a better place for it?
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.
I tried to do that and found that the command explicitly disables logging:
default_settings = {'LOG_ENABLED': False} |
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.
We might need to find another place to log these errorrs, or document it (in scrapy-spider-metadata?). But it seems this shouldn't block this PR.
OK, I guess we need a test because the initially pushed code didn't work correctly. |
try: | ||
from scrapy_spider_metadata import get_spider_metadata | ||
except ImportError: | ||
pass |
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.
It seems we lack tests on CI which check a case where the library is not installed
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.
3.6 and 3.7 :)
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.
Why isn't this line detected as covered?
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.
None of run() is detected as covered: https://app.codecov.io/github/scrapinghub/scrapinghub-entrypoint-scrapy/commit/ebf23f19ff3bc1853a35f001a8b2df7e1b1ec63b/blob/sh_scrapy/commands/shub_image_info.py
Not sure if codecov can detect coverage for code run as a separate process.
No description provided.