Skip to content

Conversation

@blerner
Copy link
Member

@blerner blerner commented Feb 14, 2025

We don't yet have any test cases that notice this change; I ran it manually on one example so far. I'll try to add some more test cases soon.

@blerner
Copy link
Member Author

blerner commented Aug 24, 2025

@dbp is this obsoleted by your brownplt/pyret-lang#1802 ? I think the place-image-align part is still useful, but your change for add-line is different from this one...

@schanzer
Copy link
Contributor

@dbp checking in about Ben's comment here.

@blerner
Copy link
Member Author

blerner commented Nov 18, 2025

I think the pinhole support is missing from the other PR? But regardless, this particular CPO PR is obsolete because the image library has moved to pyret-lang. @dbp can you confirm where both parts of your improvements are, and whether we've fixed the crashing bug in your other PR? And do we have tests for this so we can merge it?

@dbp
Copy link
Collaborator

dbp commented Nov 19, 2025

IIRC, when I changed add-line, I didn't change some other functions that were nearby, that seemed like maybe they could use refactoring, as they were implemented very differently (but, one upside meant that they didn't have the bug that add-line had). As far as I know, the bug that @jpolitz saw for command-line node-canvas still exists (unless he fixed it)?

I did not add automated test cases in that PR (not sure the workflow for doing that with image stuff? Do existing image tests test pixel output? Or compare for equality for different operations? Or?).

Not quite sure what I'm being asked here -- I do not know this code well :)

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.

4 participants