Replace checkers with validators #36

ziima · 2018-09-14T15:43:57Z

Refs #35

I've tried to transform the checkers to validators. Comments are welcome.

pawamoy · 2018-09-14T16:14:20Z

Wow. Not really available for review today, but I took a quick glance and it looks very professional as usual. I'll take the time to review asap!

ziima · 2018-09-14T16:43:50Z

No rush. I haven't yet tried it with any of our projects :)

pawamoy · 2018-09-18T18:27:38Z

README.rst

-                raise ValueError('%s must be a a string or a compiled regex '
-                                 '(use re.compile)' % name)
+                # Raise ValidationError
+                raise ValidationError('%s must be a string or a compiled regex (use re.compile)')


So, will Django populate the %s itself? I think it should be like what you wrote later:

raise ValidationError('%(value)s is not a string or a compiled regex (use re.compile)', params={'value': value})

Yes, moreover it's recommended way to raise a ValidationError, see https://docs.djangoproject.com/en/2.1/ref/forms/validation/#raising-validationerror

Can you simply copy-paste it then 🙂 ? So both examples use the recommended way.

Now I get the problem. Thanks :)

pawamoy

Great PR, thanks 🙂 Just a few questions/changes before merging, but nothing crazy.

pawamoy · 2018-09-18T18:35:03Z

docs/usage.rst


-The second way is to subclass ``appsettings.Setting`` and write a custom
-``checker`` method:
+The most customization can be reached by customizing the ``validate()`` method.


The most customization sounds wrong, maybe something like The finest-grained customization can be obtained would be better? Or maybe you forgot a word?

More complex customization can be reached?

src/appsettings/settings.py

pawamoy · 2018-09-18T18:48:47Z

src/appsettings/settings.py

+            self.validators.append(ValuesTypeValidator(item_type))
+        if empty is not None:
+            warnings.warn("Empty argument is deprecated, use min_length instead.", DeprecationWarning)
+            min_length = 1


Shouldn't we check if empty is true or false? Deprecation is a thing, but the behavior should stay the same. If the user says empty is allowed (true), then min length should be 0 (or None, to avoid adding an unnecessary validator). If not (false), then indeed min length should be 1.

My bad, looks like I got ahead of myself.

pawamoy · 2018-09-18T18:49:52Z

src/appsettings/settings.py

+            call_default=call_default, transform_default=transform_default, validators=validators)
+        if empty is not None:
+            warnings.warn("Empty argument is deprecated, use min_length instead.", DeprecationWarning)
+            min_length = 1


Same as for IterableSetting.

pawamoy · 2018-09-18T18:57:26Z

src/appsettings/settings.py

-                 **checker_kwargs):
+    default_validators = (TypeValidator(list), )
+
+    def __init__(self, name='', default=list, **kwargs):


In previous classes you didn't use **kwargs, is there a reason? I like using **kwargs because avoids duplicated code, however it can decrease the ability of introspection (IDE popup with method's accepted arguments is my main example). So I'm not sure 😄 What do you think?

I noticed, most of them now override the __init__ just to provide a different default value. I was hoping to solve that in subsequent PR. I've created #37, so I don't forget.

I have tried to use the **kwargs whenever possible as the number of arguments is quite large, and in this case I consider the duplication worse than missing introspection. In places where it's not used, new argument are appended.

Also I forgot about *args, so I wiil add them as well, in case somebody uses them.

pawamoy · 2018-09-18T18:58:21Z

src/appsettings/settings.py

+            self.validators.append(DictValuesTypeValidator(value_type))
+        if empty is not None:
+            warnings.warn("Empty argument is deprecated, use min_length instead.", DeprecationWarning)
+            min_length = 1


Same as for IterableSetting and StringSetting.

pawamoy · 2018-09-18T19:00:03Z

src/appsettings/settings.py

+            empty (bool): whether empty iterable is allowed. Deprecated in favor of min_length.
+        """
+        if key_type is not None:
+            warnings.warn("Argument key_type does nothing and is deprecated.", DeprecationWarning)


Why should it be deprecated?

I haven't found any use case nor restriction to maintain it. Objects which can serve as a key in dictionary are limited to hashables by python and I don't actually see any benefits in further restriction.

I'm not very strong about this, so if you have any arguments to keep it, I'm open to reconsider.

One could want to restrict the key type to int only:

# official django setting MESSAGE_TAGS = { messages.DEBUG: 'default', messages.INFO: 'info', messages.SUCCESS: 'success', messages.WARNING: 'warning', messages.ERROR: 'danger', }

Or any other hashable type, like a tuple.

NEURONAL_NETWORK = { (0, 0, 0): 1, (0, 0, 1): 0, ... }

Since I'm going to manipulate the keys as well, I like the possibility to enforce their type so I don't have to do it manually.

Even if I never use the key_type, I just like to know that I can. Afterall we already have a DictValuesTypeValidator, so why not a DictKeysTypeValidator as well? It's only 14 lines of code 😛

OK, The integers make a good point.

pawamoy · 2018-09-18T19:02:54Z

src/appsettings/settings.py

+            validators (list of callables): list of additional validators to use.
+            min_length (int): minimum length of the iterable (included).
+            max_length (int): maximum length of the iterable (included).
+            empty (bool): whether empty iterable is allowed. Deprecated in favor of min_length.


Do min_length and max_length make sense for the ObjectSetting? Maybe we should instead warn if they are present and pop them from the kwargs.

Indeed, I don't see any point. That also applies to empty argument.

pawamoy · 2018-09-18T19:05:07Z

src/appsettings/settings.py

-                subsetting.check()
+        super(NestedSetting, self).check()
+        for subsetting in self.settings.values():
+            subsetting.check()


Same remark as above, should we store exception messages and concatenate them before reraising?

ziima · 2018-09-19T08:41:27Z

Round 1: fixed and rebased.

pawamoy · 2018-09-19T12:34:51Z

Great thanks 🙂 Just three things left: simple copy-paste for the regex validation example in readme, your thoughts about a DictKeysTypeValidator, and warning + dict pop for min_length max_length and empty for the ObjectSetting.

ziima · 2018-09-20T09:23:01Z

I have noticed the min_length and max_length in DictSettings. Do those make any sense? Shouldn't we remove them. I can't quite imagine any meaningful usage.

ziima · 2018-09-20T09:35:55Z

Round 2: fixed and rebased.

Only the new comment about DictSettings should remain to be resolved.

pawamoy · 2018-09-22T12:46:48Z

So, do you want to remove the min_length max_length and empty args for ObjectSetting and DictSetting in this PR? Or in another?

ziima · 2018-09-24T12:42:42Z

I've already deprecated them from ObjectSetting in here, so I could do the DictSetting as well.

pawamoy · 2018-09-24T14:35:21Z

Sure, please go on!

ziima · 2018-09-25T13:46:15Z

Done. I've also added deprecation warning to TypeChecker.

pawamoy · 2018-09-25T13:55:27Z

Perfect, let's merge it!

stinovlas · 2018-12-03T13:07:45Z

I tried these custom validators and they are working great! Would it be possible to make a new release? I could use dependency_links, but having release on PyPI is much more convenient =o).

pawamoy · 2018-12-03T13:15:26Z

Great! Will do that today 🙂

pawamoy · 2018-12-03T14:10:33Z

Done 😛

stinovlas · 2018-12-03T14:12:48Z

Done stuck_out_tongue

Thank you 😉!

pawamoy reviewed Sep 18, 2018

View reviewed changes

pawamoy requested changes Sep 18, 2018

View reviewed changes

ziima force-pushed the use-validators branch from 5270586 to d33b57c Compare September 19, 2018 08:39

ziima added 2 commits September 20, 2018 11:28

Deprecate checkers in favor of validators

ae6861a

Add basic validators

1faa64b

ziima force-pushed the use-validators branch from d33b57c to 3e4c691 Compare September 20, 2018 09:35

Use validators in provided settings

d113db9

ziima force-pushed the use-validators branch from 3e4c691 to d113db9 Compare September 25, 2018 13:42

pawamoy approved these changes Sep 25, 2018

View reviewed changes

pawamoy merged commit db5f50e into pawamoy:master Sep 25, 2018

pawamoy mentioned this pull request Sep 25, 2018

Make custom checkers easier #35

Closed

ziima deleted the use-validators branch May 22, 2019 13:54

pawamoy mentioned this pull request Aug 12, 2019

Create CallableSetting #49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace checkers with validators #36

Replace checkers with validators #36

ziima commented Sep 14, 2018

pawamoy commented Sep 14, 2018

ziima commented Sep 14, 2018

pawamoy Sep 18, 2018 •

edited

Loading

ziima Sep 19, 2018

pawamoy Sep 19, 2018

ziima Sep 20, 2018

pawamoy left a comment

pawamoy Sep 18, 2018

pawamoy Sep 18, 2018

ziima Sep 19, 2018

pawamoy Sep 18, 2018

pawamoy Sep 18, 2018

ziima Sep 19, 2018

pawamoy Sep 18, 2018

pawamoy Sep 18, 2018

ziima Sep 19, 2018

pawamoy Sep 19, 2018 •

edited

Loading

ziima Sep 20, 2018

pawamoy Sep 18, 2018 •

edited

Loading

ziima Sep 19, 2018

pawamoy Sep 18, 2018

ziima commented Sep 19, 2018

pawamoy commented Sep 19, 2018

ziima commented Sep 20, 2018

ziima commented Sep 20, 2018

pawamoy commented Sep 22, 2018

ziima commented Sep 24, 2018

pawamoy commented Sep 24, 2018

ziima commented Sep 25, 2018

pawamoy commented Sep 25, 2018

stinovlas commented Dec 3, 2018

pawamoy commented Dec 3, 2018

pawamoy commented Dec 3, 2018

stinovlas commented Dec 3, 2018

Replace checkers with validators #36

Replace checkers with validators #36

Conversation

ziima commented Sep 14, 2018

pawamoy commented Sep 14, 2018

ziima commented Sep 14, 2018

pawamoy Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawamoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawamoy Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawamoy Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziima commented Sep 19, 2018

pawamoy commented Sep 19, 2018

ziima commented Sep 20, 2018

ziima commented Sep 20, 2018

pawamoy commented Sep 22, 2018

ziima commented Sep 24, 2018

pawamoy commented Sep 24, 2018

ziima commented Sep 25, 2018

pawamoy commented Sep 25, 2018

stinovlas commented Dec 3, 2018

pawamoy commented Dec 3, 2018

pawamoy commented Dec 3, 2018

stinovlas commented Dec 3, 2018

pawamoy Sep 18, 2018 •

edited

Loading

pawamoy Sep 19, 2018 •

edited

Loading

pawamoy Sep 18, 2018 •

edited

Loading