Keep Python functions with name conflict with p5.js working as expected

[isabarros]

Solve issue #90

• Add a wrapper class to python functions called PythonFunctions because I am not able to call the python methods from pyp5js.py since they conflict with the defined methods from this file
• For map and filter functions I check if there are more than two arguments passed, if so, then if the second one is a list then the python method should be called
• For set function I check if only one argument is passed then the python method should be called
• One thing that I struggled with was creating unit tests for this scenarios, since it is difficult to mock the _P5_INSTANCE, so I tested everything manually which is not ideal. Do you have any suggestions @berinhard ?

[GoToLoop]

All of those 3 methods from class PythonFunctions are static, since the self parameter is completely ignored.
So there's no actual need to have an instance of PythonFunctions in order to access them.
And to allow static access to those methods, we can re-write PythonFunctions using built-in function type():

PythonFunctions = type('PythonFunctions', (), {
    'python_map': map,
    'python_filter': filter,
    'python_set': set
})

[isabarros]

Good suggestion, thanks!

[isabarros]

Hey @GoToLoop I have tried here and strangely I am getting an error PythonFunctions.python_map is not a function. It has worked when I imported that in ipython, but when I try to import to pyp5js it fails.

[berinhard]

@isabarros this can be breaking because of how Transcrypt implements the type. Eventually is still not possible to dynamically define classes on the fly with it. I've tested the following refactoring and it worked for me:

class PythonFunctions:

    @staticmethod
    def map(*args):
        return map(*args)

    @staticmethod
    def filter(*args):
        return filter(*args)

    @staticmethod
    def set(*args):
        return set(*args)

Then you can the methods with PythonFunction.map instead of accessing this _PYTHON_INSTANCE

[berinhard]

It's safer to check if the first argument is a callable one instead of this type checking. For exemple, args[1] can be a set or a tuple and thus this code will fail. So you can change this line to:

if (len(args) > 1) and callable(args[0]):

[berinhard]

Same thing for checking for callable here.

[berinhard]

@isabarros this can be breaking because of how Transcrypt implements the type. Eventually is still not possible to dynamically define classes on the fly with it. I've tested the following refactoring and it worked for me:

class PythonFunctions:

    @staticmethod
    def map(*args):
        return map(*args)

    @staticmethod
    def filter(*args):
        return filter(*args)

    @staticmethod
    def set(*args):
        return set(*args)

Then you can the methods with PythonFunction.map instead of accessing this _PYTHON_INSTANCE

[berinhard]

@isabarros thank you so much for this PR! We're really close to finish it. I left a few comments on the args checking and also on the conversation started by @GoToLoop (hey, thanks for reviewing the PR as well!)

[berinhard]

@isabarros testing this part of the code is problematic because, in the end, it'll be transformed into JS code. So I definitelly have to create a way to test them. So far, what I'm doing is to create examples to cover the new API. Maybe this could be a good example to have:

from pyp5js import *

def setup():
    createCanvas(650, 200)

def pow(x):
    return x ** 2

def even(x):
    return not x % 2

def draw():
    conj = set(range(10))
    conj = filter(even, conj)
    conj = map(pow, conj)

    strokeWeight(5)
    for x in conj:
        point(x, height / 2)

But don't worry about this. I still have to organize the examples dir, so I can create a new one when we merge your PR in.

[GoToLoop]

... this can be breaking because of how Transcrypt implements the type. Eventually is still not possible to dynamically define classes on the fly with it. I've tested the following refactoring and it worked for me:

@berinhard, have you tested the 1 I use setattr() instead of type()?
Is setattr() supported by Transcrypt?

class PythonFunctions: pass

setattr(PythonFunctions, 'map', map)
setattr(PythonFunctions, 'filter', filter)
setattr(PythonFunctions, 'set', set)

[berinhard]

Hm, I did not test it @GoToLoop! @isabarros this is definitely better than using classmethod decorators. We can give it a try 👍

[isabarros]

@berinhard @GoToLoop setattr() and it seems to work! Thanks for the suggestion.

[isabarros]

@isabarros testing this part of the code is problematic because, in the end, it'll be transformed into JS code. So I definitelly have to create a way to test them. So far, what I'm doing is to create examples to cover the new API. Maybe this could be a good example to have:

from pyp5js import *

def setup():
    createCanvas(650, 200)

def pow(x):
    return x ** 2

def even(x):
    return not x % 2

def draw():
    conj = set(range(10))
    conj = filter(even, conj)
    conj = map(pow, conj)

    strokeWeight(5)
    for x in conj:
        point(x, height / 2)

But don't worry about this. I still have to organize the examples dir, so I can create a new one when we merge your PR in.

Seems a fair alternative. Would also add the cases where it falls into the js functions. For instance: calling map(25, 0, 100, 0, 1000) which should return 250

[berinhard]

Thanks for you PR @isabarros! I've created this issue #109 for us to better address the JS tests instead of relying on the examples to do so.