GeoJson StyleCheck

[jtbaker]

Added a check on the GeoJson class constructor to ensure that style_function and highlight_function are functions, and that they return dictionaries.

[ocefpaf]

No need for the None and caller = callers[caller_key] and we can drop the assert here in lieu of Python Exceptions. You can use .items() like:

for key, caller in callers.items():
   if not callable(caller) and not isinstance(caller(self.data['features'][0]), dict):
       raise ValueError(f'{key} must be a callable function returning a dictionary.')

[jtbaker]

Nice, I always forget about .items() on dicts. Been spending more time in the JS world lately.

Don't we need the not None check though? The default values for style_function and highlight_function are Nones right now - so they'll fail this check if unmodified, if I'm not mistaken.

Also, will our CI travis build environment accept the f-string usage? Last time I tried to commit one it caused a failure so I've been using .format() instead.

Thanks!

[ocefpaf]

Don't we need the not None check though?

if not callable(caller) should take care of that if I'm not mistaken. The caller will be None and will be True in not callable(caller), but I believe the and should be an or. We want to raise if one or the other is True, right?

Also, will our CI travis build environment accept the f-string usage? Last time I tried to commit one it caused a failure so I've been using .format() instead.

I wrote that quickly and I use f-strings everywhere, it was not meant to be a copy-n-paste review, sorry.

With that said, we will drop Python 2.7 support in the next release and we can use a f-string backport package for py35, so I'm OK if we start introducing them now. Let's just wait for @Conengmo opinion on this one 😄

[jtbaker]

The caller will be None and will be True in not callable(caller), but I believe the and should be an or. We want to raise if one or the other is True, right?

But this will pass any other uncallable object, in addition to None, I'm pretty sure - like the dictionary in #1020.

IMO there should be only two passing cases for the callers - None and functions returning dicts.

Calling an uncallable (like None(self.data['features'][0])) raises TypeError. So we need the and operator to take the False from callable if our second call results in that TypeError, and still raise our custom ValueError.

So we need:

if caller is not None:
    if not callable(caller) or not isinstance(caller(self.data['features'][0]), dict):
        raise ValueError('Pass a function returning a dictionary to `{}`'.format(key))

Or maybe pack all the conditionals into one line? I know it is long, but I want to make it as airtight as possible and give useful feedback to the user. Let me know what you think.

[Conengmo]

If you do the validation after the lines where self.style_function and the other one are set, you don't have to check for None. But if you validate the class input then I suppose you do need to check for None because None is a valid input so it shouldn't raise an error.

I think your check is good, using callable() and checking whether normal use returns a dictionary. I'm not a fan though of putting stuff in a container and then looping over that when you want to do the same thing multiple times. In these situations a function that you call multiple times is better IMO. For example:

self._validate_function(self.style_function, 'style_function')
self._validate_function(self.highlight_function, 'highlight_function')

def _validate_function(self, func, name):
    """Check if style_function and highlight_function work as intended."""
    if not callable(func) or not isinstance(func(self.data['features'][0]), dict):
        raise ValueError()

But note that I don't mind keeping it as it is. I'm interested though in what you think is cleaner code.

Another idea to make it less convoluted is to maybe not make a dictionary to loop over:

for func_name in ('style_function', 'highlight_function'):
    func = getattr(self, func_name)
    # do the check

[Conengmo]

I added my take on your discussion. I guess I'm not helping by adding more text :) But it is interesting to hear your thoughts on code style.

By the way the Travis build is failing on a single notebook related test:
https://travis-ci.org/python-visualization/folium/jobs/459020933#L2934

[Conengmo]

If you do the validation after the lines where self.style_function and the other one are set, you don't have to check for None. But if you validate the class input then I suppose you do need to check for None because None is a valid input so it shouldn't raise an error.

I think your check is good, using callable() and checking whether normal use returns a dictionary. I'm not a fan though of putting stuff in a container and then looping over that when you want to do the same thing multiple times. In these situations a function that you call multiple times is better IMO. For example:

self._validate_function(self.style_function, 'style_function')
self._validate_function(self.highlight_function, 'highlight_function')

def _validate_function(self, func, name):
    """Check if style_function and highlight_function work as intended."""
    if not callable(func) or not isinstance(func(self.data['features'][0]), dict):
        raise ValueError()

But note that I don't mind keeping it as it is. I'm interested though in what you think is cleaner code.

Another idea to make it less convoluted is to maybe not make a dictionary to loop over:

for func_name in ('style_function', 'highlight_function'):
    func = getattr(self, func_name)
    # do the check

[jtbaker]

If you do the validation after the lines where self.style_function and the other one are set, you don't have to check for None. But if you validate the class input then I suppose you do need to check for None because None is a valid input so it shouldn't raise an error.

What is the best design pattern here? To me, it seems like it would be best to head off these errors before assigning to self. But I guess if we're running the validation inline toward the bottom of the constructor that could work, too.

Whatever makes it easiest for maintainers down the line to understand/make changes as the library evolves.

@ocefpaf what do you think?

[Conengmo]

What is the best design pattern here?

I guess if we can't choose, then it doesn't matter what we choose :)

[ocefpaf]

I guess if we can't choose, then it doesn't matter what we choose :)

😄

What is the best design pattern here?

More inclined to follow the pattern where you don't have to check for None.

[jtbaker]

I guess if we can't choose, then it doesn't matter what we choose :)

Haha, I wasn't sure if there was a 'pythonic' or OOP best practice way to manage that kind of stuff.

Sounds good - I'll go ahead and rewrite it to a _validate_function structure and call it after assignment.

[jtbaker]

By the way the Travis build is failing on a single notebook related test:
https://travis-ci.org/python-visualization/folium/jobs/459020933#L2934

Looks like I was able to fix the failing test by handling the case where self.data is a single GeoJson feature instead of a FeatureCollection.

[Conengmo]

Good stuff, thanks @jtbaker! And thanks @ocefpaf for reviewing this.