Repository navigation
Reject bool for int settings (CACHE_SIZE_LIMIT, DEFAULT_DAYS_IN_MONTH) - #1386
Pitchfork-and-Torch wants to merge 1 commit into
Conversation
serhii73
left a comment
There was a problem hiding this comment.
Checked the diff, the surrounding code and the docs. I'm not approving yet, because one issue should be settled first.
URGENT: breaking change for callers that pass a bool (dateparser/conf.py:263-267, the new isinstance(setting_value, bool) branch).
CACHE_SIZE_LIMIT treats any falsy value as "no limit" (dateparser/languages/dictionary.py:314-315: self._settings.CACHE_SIZE_LIMIT and len(cache) > ...), and docs/settings.rst says to set 0 to turn the limit off. CACHE_SIZE_LIMIT=False is not documented, but it works today and means exactly that: on current master DateDataParser(settings={"CACHE_SIZE_LIMIT": False}) is accepted and parse("12 June 2020", settings={"CACHE_SIZE_LIMIT": False}) returns a date. With this PR the same call raises SettingValidationError, so code that runs fine on 1.5.0 breaks on upgrade. The same applies to True/False for DEFAULT_DAYS_IN_MONTH. The project usually deprecates before it rejects (for example #1556).
Suggested fix: keep accepting bool for now, convert it with int() and emit a DeprecationWarning that says to pass an int (0 to disable the cache limit), and reject it in a later release. At the very least, document the new validation and keep False working as 0 for CACHE_SIZE_LIMIT.
Non-urgent notes:
- No usable CI: the three workflow runs for this head ended in
failurewith no jobs (a startup failure), and the branch conflicts with current master. Please rebase so CI can run. - The test asserts only that valid values give a truthy object. Assert the stored value (
settings.CACHE_SIZE_LIMIT == 100, and0).assertRaisesRegexis given an unescaped pattern (.and quotes);re.escapewould make it robust. - Only
boolis rejected;DEFAULT_DAYS_IN_MONTH=0/negative and a negativeCACHE_SIZE_LIMITare still accepted. Optional follow-up. setting_props["type"] is intwould not cover a future setting declared as a tuple of types ornumbers.Integral; a check of the form "bool value andboolnot among the expected types" would be more general.
CACHE_SIZE_LIMIT=False is still stored as 0, which disables the limit, and True is stored as 1. DEFAULT_DAYS_IN_MONTH is the same. Both emit DeprecationWarning. A later release can reject them.
c881cc8 to
f3b397a
Compare
|
Rebased onto current master. Bool is no longer rejected. CACHE_SIZE_LIMIT=False is stored as 0, which still disables the limit, and True is stored as 1. DEFAULT_DAYS_IN_MONTH is the same. Both emit DeprecationWarning, and the docs and HISTORY say a later release can reject them. The tests assert the stored int, including 0 and 100. |
Summary
boolsubclassesint, soisinstance(True, int)is true. That letCACHE_SIZE_LIMIT=True/DEFAULT_DAYS_IN_MONTH=Truepass settings validation and silently behave as1(andFalseas0).Reject
boolexplicitly when the setting type isint.Test plan
test_check_settings_rejects_bool_for_int_settingspassesCACHE_SIZE_LIMIT/DEFAULT_DAYS_IN_MONTHstill accepted