search

defun-games / claylib

A Common Lisp 2D/3D game toolkit built on top of Raylib 4.5.
zlib License
69 stars 4 forks source link

Add PRINT-OBJECT for all classes #66

Closed simendsjo closed 1 year ago

simendsjo commented 1 year ago

Closes #65

simendsjo commented 1 year ago

Might be nice to avoid the need for the empty definitions that aren't adding new slots to print, but that depends on how comfortable you are with macros.

I thought about it, it figured it would obfuscate the usage somewhat. Now it's more explicit that it takes a list of methods, and it's ready to accept some input (which is also why i added the list on it's own line).

shelvick commented 1 year ago

Aha, so between your PR and now, I merged linking into main, which added a new parent class to nearly all Claylib classes. Adding (define-print-object linkable (children)) will fix the clutter, however children is a hash table. This could be somewhat risky to print in full by default because it itself will contain other objects. I'd suggest printing NIL if no children and just the hash table object if there are children -- or perhaps print the number of children. I think actually listing out the children will dictate a formatting change and we should save that for later.

simendsjo commented 1 year ago

This could be somewhat risky to print in full by default because it itself will contain other objects.

Hash tables don't print the content, only length :CHILDREN #<HASH-TABLE :TEST EQL :COUNT 0 {100BA7FD43}>. I'll add a new PR.

simendsjo commented 1 year ago

I'll add a new PR.

Oops, sorry :/ I thought the PR was merged. So I managed to close it by deleting the branch, and "Reopen" is unavailable (maybe it needs elevated permissions). So I added a new PR, #69.

You might notice that I keep rebasing to squash my changes. This is not something I'm very fond of, but many people merge once the PR is in good shape before I get the chance to squash changes, leaving lots of half-baked commits in the history. This makes following changes in the PR more difficult, so if you don't merge before squashing commits, I'll start adding the individual changes as new commits to ease the code review process.

shelvick commented 1 year ago

Hash tables don't print the content, only length

Not by default, but you could, with e.g. alexandria:hash-table-plist. It's a matter of whether that's a good idea (which in this case I don't think it is).

many people merge once the PR is in good shape before I get the chance to squash changes

Well, now that I know this, I can simply wait until you give the OK. :wink:

On GitHub you can also do a draft PR for larger changes which indicates that it's only for discussion and not ready for merge yet. mjkalyan and I have done that several times here.