Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
moldova: add from_date support #701
moldova: add from_date support #701
Changes from 3 commits
5101165
041bd26
8bd8997
bd6d141
af4da5e
e5bae22
3641330
50f709e
fcaacfe
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 suggest 2018-01-01 (we can even do 2010-01-01), since we don't need to have the exact from date for this API. We should only set a precise from date if the API will error otherwise. That way, if the API fills in historical data, we don't accidentally omit it.
I think the afghanistan spiders are also too precise.
There are some that have a precise month - I don't know if they can be relaxed as well: honduras_portal_bulk, malta, uruguay_base, zambia.
We can update the docstring about setting
default_from_date
.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.
Actually, I didn't want to add a default_date for this one, as it is not required, but
kingfisher-collect/kingfisher_scrapy/commands/checkall.py
Line 121 in 81e9a22
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.
That check is there, because otherwise how will the f-string be formatted?
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 default_from_date is used in:
kingfisher-collect/kingfisher_scrapy/commands/checkall.py
Lines 160 to 165 in 81e9a22
kingfisher-collect/kingfisher_scrapy/commands/checkall.py
Line 182 in 81e9a22
but I think that if
date_required
is false and onlyfrom_date
or onlyuntil_date
is implemented, then we don't need adefault_from_date
or adefault_until_date
, right?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.
Yes, it's not required in that specific scenario.
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.
So maybe I can update this check and remove
default_from_date
from Moldova?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.
Sounds good, but I think the f-string also needs to be updated as right now it would format it with
None
, no?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.
Done, and I also added that the check ask if there is a default_until_date
I also updated all the warnings that we had
Honduras and Uruguay are fine as they have fixed dates that are not going to change. For Afghanistan, zambia and Malta, the spider ask for both dates to be set (from_date and until_date) and I think that is not required, but I will update that as part of #600