Repository navigation
Locale order support - #789
Conversation
| if not locales and use_given_order: | ||
| raise ValueError("locales must be given if use_given_order is True") | ||
| if not (locales or languages) and settings.USE_GIVEN_LANGUAGE_ORDER: | ||
| raise ValueError("locales or languages must be given if USE_GIVEN_LANGUAGE_ORDER is True") |
There was a problem hiding this comment.
Not sure of this failure. If the given list of languages is empty in this case then it should use the defaults.
I'm thinking on the case where this is combined with a language detection model. For some cases, the prediction could fail and not return any language, but still, dateparser must try its best.
There was a problem hiding this comment.
I do see your point, and I agree. The question is if this is what's expected: to restrict to the given locales (or languages) or to use those as a starting point but try the other locales anyways.
If the second is true, then removing this check should be the right choice.
There was a problem hiding this comment.
I can talk about the use case I have in mind: using a page language detector to provide hints to have better date parsing. In this case, what you have is just hints:
- In some case, you don't get any language from the language detector. Still, you want dateparser to try its best.
- In some cases, the list won't be complete. You don't want to remove locales just because the language detector didn't detect them.
So from my point of view, this list should be just hints: they can change the order of the default locales, but never restrict them.
There was a problem hiding this comment.
Hi @ivanprado, the languages and locales parameters have been used historically for restricting the used languages. This is sometimes really important and helps to improve the performance, so I think we can't change them to work as you mentioned.
However, what you say sounds like adding something new like a exclude_languages parameter, so you could do something like this:
date = dateparser.parse("<date string>", languages=['ru', 'tr', 'es'])
if not date:
date = dateparser.parse("<date string>", exclude_languages=['ru', 'tr', 'es'])What do you think? Could this work for you?
There was a problem hiding this comment.
Also, on the argument of not raising an exception because the calling code may provide an empty list, the calling code should also be able not to set that setting when that happens. I fail to see the point of removing the exception.
There was a problem hiding this comment.
@noviluni I see your point. It was not clear to me that locales and languages were used as contraints to improve performance. The documentation (https://github.com/scrapinghub/dateparser/blob/master/dateparser/__init__.py#L23-L31) is ambiguous, and my first thought was that they would be used as "hints" . I think it would be great to update the documentation to make it clear that they restrict the languages used.
Given that languages and locales are for restricting I agree with you than then maybe is not the best place for the use case I'm proposing. What I'm proposing is to be able to propose "hints" for the locales/languages. I don't think exclude_languages should be way.
What do you think about a new parameter language_hints? It would be used just to alter the languages/locales priorities.
noviluni
left a comment
noviluni
left a comment
There was a problem hiding this comment.
Hey @mirceachira! Good job!
It seems that there are some errors in the flake8 pipeline: https://travis-ci.org/github/scrapinghub/dateparser/jobs/728909794, could you fix them?
|
|
||
| @apply_settings | ||
| def __init__(self, languages=None, locales=None, region=None, try_previous_locales=False, | ||
| use_given_order=False, settings=None): |
There was a problem hiding this comment.
we should probably deprecate this instead of just removing it, but maybe we can do it as we are going to release a new version with multiple breaking changes. cc: @Gallaecio
| 'RETURN_TIME_AS_PERIOD': False, | ||
| 'PARSERS': default_parsers, | ||
| 'REQUIRE_PARTS': [], | ||
| 'USE_GIVEN_LANGUAGE_ORDER': False |
There was a problem hiding this comment.
I'm not sure if it's better to keep the False by default or to change to True. Any strong opinion?
There was a problem hiding this comment.
I think False is right as a default because that way you'd get the added benefit of trying your most likely language first. If you give a list of locales I'd assume you want the date to be extracted correctly regardless of which of your locales is the right one. Also, I would argue that expecting it to be as efficient as possible by default is better if you allow the customization.
2d534ae to
09e9dd1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #789 +/- ##
=======================================
Coverage 97.11% 97.11%
=======================================
Files 235 235
Lines 2912 2913 +1
=======================================
+ Hits 2828 2829 +1
Misses 84 84 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… in DateDataParser
…ty (#770) Reconcile the rebased work onto current master with the agreed design: - Keep the public ``use_given_order`` constructor argument (non-breaking); the new ``USE_GIVEN_LANGUAGE_ORDER`` setting is OR-ed with it rather than replacing it, and also reaches the top-level ``dateparser.parse()``. - When the setting is True but no languages/locales are given, it is a no-op (fall back to the default order) instead of raising. - Register ``USE_GIVEN_LANGUAGE_ORDER`` in ``check_settings`` so it is a valid setting, document it under "Language Order" in docs/settings.rst, and add settings-validation and parsing tests. - Revert the unneeded loader change: DateDataParser already normalizes an empty languages list to None. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
94c96fd to
cc3a925
Compare
closes #770