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

Allow to use_given_order for languages too #997

Merged

Conversation

whalebot-helmsman
Copy link
Member

@whalebot-helmsman whalebot-helmsman commented Oct 7, 2021

No description provided.

@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #997 (585a8db) into master (41f9478) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #997   +/-   ##
=======================================
  Coverage   98.29%   98.29%           
=======================================
  Files         234      234           
  Lines        2694     2694           
=======================================
  Hits         2648     2648           
  Misses         46       46           
Impacted Files Coverage Δ
dateparser/date.py 99.24% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41f9478...585a8db. Read the comment docs.

dateparser/date.py Outdated Show resolved Hide resolved
@Gallaecio
Copy link
Member

Is that all that needs changing? (i.e. was the feature there but this exception was getting in the way?)

Could you add a test?

@whalebot-helmsman
Copy link
Member Author

Is that all that needs changing? (i.e. was the feature there but this exception was getting in the way?)

Yes, e.g. for French format is dd/mm and for English it is mm/dd . English is used as a fallback, so we always add it as last one. You don't have a list of locales, just these two languages. You try to use class and get mm/dd .

To solve it I wrote this class

class OrderedDateDataParser(DateDataParser):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.use_given_order = True

And everything starts working

Could you add a test?

There is a test of loading languages with use_given_order

param(given_languages=['es', 'fr', 'en'],
given_locales=None,
given_region=None,
loaded_languages=['en', 'es', 'fr'],
loaded_locales=['en', 'es', 'fr'],
expected_locales=['es', 'fr', 'en']),

@Gallaecio
Copy link
Member

There is a test of loading languages with use_given_order

But it was not failing before. We need a test that would have caught the issue before your change, i.e. a test that fails if we revert your change, and passes after we apply it. And I don’t mean https://github.com/scrapinghub/dateparser/pull/997/files#diff-fd61b87100377a706498644e5ff42b8baecf18918169f6d4082af10d5a053477R658.

@Gallaecio Gallaecio merged commit 5ec1a6e into scrapinghub:master Oct 27, 2021
10 checks passed
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.

None yet

2 participants