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.

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

Merged
merged 9 commits into from
Apr 21, 2021
Merged

moldova: add from_date support #701

merged 9 commits into from
Apr 21, 2021

Conversation

yolile
Copy link
Member

@yolile yolile commented Apr 20, 2021

ref #346

@yolile yolile requested a review from jpmckinney April 20, 2021 20:39
kingfisher_scrapy/base_spider.py Outdated Show resolved Hide resolved
"""
name = 'moldova'

# SimpleSpider
data_type = 'release_package'

# BaseSpider
date_format = 'datetime'
default_from_date = '2018-10-18T00:00:00'
Copy link
Member

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.

Copy link
Member Author

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

self.check_date_spider_argument('from_date', spider_arguments, lambda cls: repr(cls.default_from_date),
fails otherwise.

Copy link
Member

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?

Copy link
Member Author

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:

if self.cls.date_required:
format_string += " Defaults to {default}."
elif spider_argument == 'from_date':
format_string += "\n If ``until_date`` is provided, defaults to {default}."
elif spider_argument == 'until_date':
format_string += "\n If ``from_date`` is provided, defaults to {default}."

expected = format_string.format(period=period, format=format_, default=default(self.cls))

but I think that if date_required is false and only from_date or only until_date is implemented, then we don't need a default_from_date or a default_until_date, right?

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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?

Copy link
Member Author

@yolile yolile Apr 21, 2021

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?

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

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.

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

@yolile
Copy link
Member Author

yolile commented Apr 21, 2021

@jpmckinney now the coverage is failing because of checkall but I think that is ok

Copy link
Member

@jpmckinney jpmckinney left a comment

Choose a reason for hiding this comment

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

I made some changes. Please have a look and feel free to merge.

@jpmckinney
Copy link
Member

@jpmckinney now the coverage is failing because of checkall but I think that is ok

That's fine, it is not a required check.

@yolile yolile merged commit b990c7f into main Apr 21, 2021
@yolile yolile deleted the 346-moldova branch April 21, 2021 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants