Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

str and repr implementation for the py5 class. #251

Merged
merged 10 commits into from
Mar 11, 2023
Merged

str and repr implementation for the py5 class. #251

merged 10 commits into from
Mar 11, 2023

Conversation

AsadNizami
Copy link
Contributor

Fix for #209. Tested the changes. I'll implement these methods for other classes also if this looks fine. Please let me know if there are any changes to make.

@hx2A
Copy link
Collaborator

hx2A commented Mar 10, 2023

Thank you, @AsadNizami , for this PR. It is off to a good start. I like how you put these new methods right after the __new__() methods of each class. That helps keep everything organized.

Opinions on how to implement str and repr methods vary somewhat, but I like to have something display that is related to the instance in some way so that if multiple are printed out, they aren't all the same. Something like this:

class Person:
    
    def __init__(self, name, age):
        self.name = name
        self.age = age

    def __str__(self):
        return f"Person(name='{self.name}', age={self.age})"

    def __repr__(self):
        return self.__str__()

Observe that __repr__() calls __str__(), so I don't have to duplicate code. If this is printed out, it will be something like Person(name='Pat', age=42). That's very readable and much better than the default Python object method, which would be something like <__main__.Person at 0x7f9664f36190>.

(As an aside, observe that Person(name='Pat', age=42) is also valid Python code that, if executed, would create a new instance of class Person. That feature could never work for the creation of py5 classes but that has been useful to me in other situations, and is an idea for you to keep in your back pocket for another day.)

Anyhow, back to py5. The py5 classes are more complex in that it isn't totally obvious about what should be displayed. I would look into the various methods to see if there is anything interesting or relevant to display to the user.

For Py5Font there are get_name() and get_size() methods, so perhaps Py5Font(name='Comic Sans', size=16) would be appropriate. For Sketch, Py5Graphics, and Py5Image, I would maybe go with height and width, ie Py5Image(width=200, height=200). Py5Shape has a get_name() method, but note that it might be None. If it is, then show Py5Shape(name=None) instead of Py5Shape(name='robot').

For Py5Surface and Py5Shader, I can't really think of anything right now. How about using the builtin Python method id()? So, Py5Surface(id=45345345345), where the big number comes from id(self)? At least it won't be the same for different objects. I'm open to better ideas here.

@AsadNizami AsadNizami marked this pull request as draft March 10, 2023 10:32
@AsadNizami
Copy link
Contributor Author

Thanks for the feedback. I am facing some issues with string formatting when implementing the __str__ method.
I am getting IndexError with the following changes:

class Py5Font:
    """$classdoc_Py5Font
    """
    _cls = jpype.JClass('processing.core.PFont')
    CHARSET = _cls.CHARSET

    _py5_object_cache = weakref.WeakSet()

    def __new__(cls, pfont):
        for o in cls._py5_object_cache:
            if pfont == o._instance:
                return o
        else:
            o = object.__new__(Py5Font)
            o._instance = pfont
            cls._py5_object_cache.add(o)
            return o

    def __str__(self):
        return "Py5Font(name='{}', size={})".format(self.get_name(), self.get_size())
...
2023-03-10 19:23:42,159 | INFO | copying py5_resources/py5_module/py5/__init__.py to ../py5code/py5/__init__.py
2023-03-10 19:23:42,175 | INFO | copying py5_resources/py5_module/py5/render_helper.py to ../py5code/py5/render_helper.py
2023-03-10 19:23:42,177 | INFO | copying py5_resources/py5_module/py5/mouseevent.py to ../py5code/py5/mouseevent.py
2023-03-10 19:23:42,177 | INFO | copying py5_resources/py5_module/py5/surface.py to ../py5code/py5/surface.py
2023-03-10 19:23:42,178 | INFO | copying py5_resources/py5_module/py5/font.py to ../py5code/py5/font.py
Traceback (most recent call last):
  File "generate_py5.py", line 239, in <module>
    main()
  File "generate_py5.py", line 235, in main
    generate_py5(args.processing_app_dir, args.py5_build_dir, args.skip_autopep8)
  File "generate_py5.py", line 206, in generate_py5
    shutil.copytree(Path('py5_resources', 'py5_module'), build_dir, copy_function=copier, dirs_exist_ok=True)
  File "/home/asad/anaconda3/envs/py5/lib/python3.8/shutil.py", line 557, in copytree
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
  File "/home/asad/anaconda3/envs/py5/lib/python3.8/shutil.py", line 495, in _copytree
    copytree(srcobj, dstname, symlinks, ignore, copy_function,
  File "/home/asad/anaconda3/envs/py5/lib/python3.8/shutil.py", line 557, in copytree
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
  File "/home/asad/anaconda3/envs/py5/lib/python3.8/shutil.py", line 499, in _copytree
    copy_function(srcobj, dstname)
  File "/home/asad/Documents/projects/pythonprocessing/py5generator/generator/util.py", line 49, in __call__
    content = content.format(**self.format_params)
IndexError: Replacement index 0 out of range for positional args tuple
make: *** [Makefile:25: ../py5code] Error 1

KeyError with the other way of string formatting.

class Py5Font:
    """$classdoc_Py5Font
    """
    _cls = jpype.JClass('processing.core.PFont')
    CHARSET = _cls.CHARSET

    _py5_object_cache = weakref.WeakSet()

    def __new__(cls, pfont):
        for o in cls._py5_object_cache:
            if pfont == o._instance:
                return o
        else:
            o = object.__new__(Py5Font)
            o._instance = pfont
            cls._py5_object_cache.add(o)
            return o

    def __str__(self):
        return f"Py5Font(name='{self.get_name()}', size={self.get_size()})"
    
2023-03-10 19:29:30,890 | INFO | copying py5_resources/py5_module/py5/mouseevent.py to ../py5code/py5/mouseevent.py
2023-03-10 19:29:30,891 | INFO | copying py5_resources/py5_module/py5/surface.py to ../py5code/py5/surface.py
2023-03-10 19:29:30,892 | INFO | copying py5_resources/py5_module/py5/font.py to ../py5code/py5/font.py
Traceback (most recent call last):
  File "generate_py5.py", line 239, in <module>
    main()
  File "generate_py5.py", line 235, in main
    generate_py5(args.processing_app_dir, args.py5_build_dir, args.skip_autopep8)
  File "generate_py5.py", line 206, in generate_py5
    shutil.copytree(Path('py5_resources', 'py5_module'), build_dir, copy_function=copier, dirs_exist_ok=True)
  File "/home/asad/anaconda3/envs/py5/lib/python3.8/shutil.py", line 557, in copytree
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
  File "/home/asad/anaconda3/envs/py5/lib/python3.8/shutil.py", line 495, in _copytree
    copytree(srcobj, dstname, symlinks, ignore, copy_function,
  File "/home/asad/anaconda3/envs/py5/lib/python3.8/shutil.py", line 557, in copytree
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
  File "/home/asad/anaconda3/envs/py5/lib/python3.8/shutil.py", line 499, in _copytree
    copy_function(srcobj, dstname)
  File "/home/asad/Documents/projects/pythonprocessing/py5generator/generator/util.py", line 49, in __call__
    content = content.format(**self.format_params)
KeyError: 'self'
make: *** [Makefile:25: ../py5code] Error 1

I'll be grateful if you can help me here.

@hx2A
Copy link
Collaborator

hx2A commented Mar 10, 2023

Oh, I see. I forget about this issue all the time.

To understand the issue, first I should explain how py5generator works. It is basically a custom template engine that uses data and templates to create the actual py5 code.

All of the files in py5_resources/py5_module are the templates. The data comes from csv files, the output of javap (to inspect the Processing Jars), and the reference documentation files.

The file that you are editing, font.py, is itself a string that gets used as a format string. The {py5font_class_members_code} you see at the end is one of the keys. The py5generator code creates the Py5Font code that gets inserted in that location of the file. That file is also valid Python code that passes a linter. That's what the py5font_class_members_code = None # DELETE is doing at the top of the file, to make sure that py5font_class_members_code is defined.

All of this works great but it gets tripped up if I try to write code in a template that uses { } anywhere. That is what is happening here. The solution is to either use string concatenation (ie "Py5Font(name='" + self.get_name() + ...) or to use double curly braces return f"Py5Font(name='{{self.get_name()}}', size={{self.get_size()}})". Probably the later is simpler in this case.

@AsadNizami AsadNizami marked this pull request as ready for review March 10, 2023 16:43
@AsadNizami
Copy link
Contributor Author

I've made the changes, please review them.

@AsadNizami AsadNizami marked this pull request as draft March 10, 2023 17:36
@AsadNizami AsadNizami marked this pull request as ready for review March 10, 2023 17:39
@hx2A
Copy link
Collaborator

hx2A commented Mar 10, 2023

It is looking pretty good! Thank you for your efforts.

I appreciate the -> str typehints. Can you make sure all of them have that? For Py5Shape, did you test to see what happens if the name is None? Can you make that f"Py5Shape(name='{{self.get_name()}})'"?

Also, Py5Graphics and Py5Image (in graphics.py and image.py) should be similar to what you added to sketch.py.

@AsadNizami
Copy link
Contributor Author

Please tell me how to create objects for those classes requiring additional arguments in the __new__ method like. Also, are there other ways to test these classes besides running the GUI?

@hx2A
Copy link
Collaborator

hx2A commented Mar 10, 2023

There's no way to create any of these objects without creating a py5 Sketch and running it.

@AsadNizami
Copy link
Contributor Author

Ok, thanks!

@AsadNizami AsadNizami marked this pull request as draft March 11, 2023 07:48
@AsadNizami
Copy link
Contributor Author

AsadNizami commented Mar 11, 2023

I've made the changes. When I tested the classes, I noticed that for some of the classes, the 'double parenthesis' syntax was printed as {self.get_name()} and not its value, so I implemented it this way. It's working fine for the Py5Shape class even when the object is None.

Screenshot from 2023-03-11 13-55-21

@AsadNizami AsadNizami marked this pull request as ready for review March 11, 2023 08:31
@hx2A
Copy link
Collaborator

hx2A commented Mar 11, 2023

Thank you, @AsadNizami ! Almost there, just a few minor tweaks.

  • I see you accidentally added a file hs_err_pid32804.log to the PR. You must have had a core dump somewhere? Can you remove this?
  • It just occurred to me that width before height is probably a better ordering because that's the order those two values appear in py5 and Processing methods. Can you switch that for Sketch, Py5Image, and Py5Graphics?
  • I know some people like using \ at the end of a line to indicate code continuation but for me that's a pet peeve. I'm not that concerned about line widths or 80 characters or any of that. If a line of code not much longer than the longest line in each file, you can put it in one line. If it is, I'd prefer parentheses instead.
  • There's some extra whitespace characters that can be removed. Note that VSCode has a Toggle Render Whitespace command (which I have on all the time because that's my preference)

@AsadNizami AsadNizami marked this pull request as draft March 11, 2023 12:44
@AsadNizami
Copy link
Contributor Author

Please review

@AsadNizami AsadNizami marked this pull request as ready for review March 11, 2023 13:07
@hx2A
Copy link
Collaborator

hx2A commented Mar 11, 2023

This is great, @AsadNizami , thank you for your perseverance and for working with me to create a solid PR. I will merge this now...

@hx2A hx2A merged commit 44d3217 into py5coding:main Mar 11, 2023
@AsadNizami
Copy link
Contributor Author

Thank you for helping me through this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants