[wheevu]

Resolves #94

Adds output_type='text/plain' to both draw() APIs.
Text is returned as a string or written to a file.
Images stay the default, and ascii() still works.

Type of change

• Bug fix
• New feature
• Breaking change
• This change requires a documentation update

Changes

Added

• tests/test_express_api_draw_text.py covers text output, files, errors, and compatibility.
• tests/test_express_api_draw_documentation.py keeps private helpers out of the API docs.

Updated

• Drawing APIs, README, and examples support text output.
• ascii.py is renamed to draw_as_text.py.

Testing

• pytest -vv: 118 passed on Python 3.11 and 3.14, macOS ARM64.
• Examples, docs, and 12 compatibility checks pass.

Linux, Windows, Python 3.12, and Python 3.13 were not tested locally.

[wbkboyer]

Hey @wheevu ! Thank you for your contribution; after merging in master, I made a few changes to the original #94 spec and tweaked a few things (see commit message for 8a034c).

I'm going to have to pick up this review on Monday just to make sure I understand the added tests, because in particular I'm a bit confused by the check for the outfileName that you're doing in PGraph._draw_as_text():

        if outfileName is not None and '\0' in outfileName:
            raise ValueError("planarity: outfileName must not contain a NUL.")

Can you verify that you're trying to ensure that if an outfileName is given, that it is a valid str rather than a null-terminated char *, because otherwise the line encoded_filename = outfileName.encode('utf-8') will fail?

Thank you for your patience, and again, thank you for tackling this work! 🎉

[wheevu]

Hi @wbkboyer! Thank you for the warm welcome! 😄

The check prevents filename truncation in the C layer.
outfileName is already declared as a Python str, and .encode('utf-8') preserves embedded NUL characters.

Without the check, 'notes.txt\0.bak' would reach gp_DrawPlanar_RenderToFile(), where fopen() would interpret it as 'notes.txt' and could overwrite that file. Raising ValueError should prevent this and would match Python's open() behavior.

test_text_rejects_embedded_nul_filename checks that the call raises and the existing file stays unchanged.

Hope that clears things up a bit. Thanks again for your time!

[wbkboyer]

Hi @wbkboyer! Thank you for the warm welcome! 😄

The check prevents filename truncation in the C layer. outfileName is already declared as a Python str, and .encode('utf-8') preserves embedded NUL characters.

Without the check, 'notes.txt\0.bak' would reach gp_DrawPlanar_RenderToFile(), where fopen() would interpret it as 'notes.txt' and could overwrite that file. Raising ValueError should prevent this and would match Python's open() behavior.

test_text_rejects_embedded_nul_filename checks that the call raises and the existing file stays unchanged.

Hope that clears things up a bit. Thanks again for your time!

Oh right the heck on, that makes sense that since both \ and 0 are valid characters, that some Python string containing \0 (which is considered as a single char according to len()) might be sent in as the outfileName, which would then get truncated at the C-layer! I didn't realize that passing in a string containing \0 would be accepted by Python and that it would be treated as just a normal character:

>>> foo = "hello\0world"
>>> print(foo)
helloworld
>>> len(foo)
11
>>> foo
'hello\x00world'

(For my future reference) Python Null Character (\0) in Strings contains a more in-depth discussion of the \0 char.

I think there's a couple other places this same safeguard (e.g. write() method within PGraph, as well as members of the full API), but that can be the subject of a future issue.

I've made a couple other fixes to docstrings in efae7c that I overlooked in 8a034c(in spite of my process of iteratively rebuilding the docs locally any time I changed the sourcefiles :S D'OH), and added DeprecationWarning-type warnings.warn() to the ascii() method and function (which I also made sure that pytest ignores with the added config in pyproject.toml).

I still want to double-check the tests; thanks for your patience with this process! 🎉 <3

[wbkboyer]

Thank you again for your patience with this process, @wheevu ! I hope you can forgive me for all the suggested test name changes; I was trying to make sure they're consistent, and that they indicate precisely which path they're hitting, but please let me know what you think (e.g. more concise, consistent, and descriptive alternatives) 🎉

Also, if you think there's any subtleties that are hard to succinctly capture in the test names, may you please add short docstring that describes the various parameterizations or any assumptions that might be baked into the test?

[john-boyer-phd]

Thank you again for your patience with this process, @wheevu ! I hope you can forgive me for all the suggested test name changes; I was trying to make sure they're consistent, and that they indicate precisely which path they're hitting, but please let me know what you think (e.g. more concise, consistent, and descriptive alternatives) 🎉

Also, if you think there's any subtleties that are hard to succinctly capture in the test names, may you please add short docstring that describes the various parameterizations or any assumptions that might be baked into the test?

Hi Josh (@wheevu), Wanda and I have talked through the comments, and Wanda is just going to make a few small changes documented in the comments, to expedite completion of this PR. I think we should leave out the docstring requirement for now because it's not on any of the other tests, and in these tests we have a good idea of what is being tested from the good method names being used. There were some very cool tests in here, Josh (also noted in the comments, some of which you'll have to dig into because the conversations are resolved). Thank you for taking on this enhancement (and the issues in your other PRs).

[wheevu]

Thank you again for your patience with this process, @wheevu ! I hope you can forgive me for all the suggested test name changes; I was trying to make sure they're consistent, and that they indicate precisely which path they're hitting, but please let me know what you think (e.g. more concise, consistent, and descriptive alternatives) 🎉
Also, if you think there's any subtleties that are hard to succinctly capture in the test names, may you please add short docstring that describes the various parameterizations or any assumptions that might be baked into the test?

Hi Josh (@wheevu), Wanda and I have talked through the comments, and Wanda is just going to make a few small changes documented in the comments, to expedite completion of this PR. I think we should leave out the docstring requirement for now because it's not on any of the other tests, and in these tests we have a good idea of what is being tested from the good method names being used. There were some very cool tests in here, Josh (also noted in the comments, some of which you'll have to dig into because the conversations are resolved). Thank you for taking on this enhancement (and the issues in your other PRs).

Thanks, John and Wanda! I'll take some time to read through all the comments properly. There's a lot for me to learn from the discussion. I really appreciate you both working through this and taking care of the final changes. Glad the tests were useful! 😄

[wbkboyer]

Thank you again for your patience with this process, @wheevu ! I hope you can forgive me for all the suggested test name changes; I was trying to make sure they're consistent, and that they indicate precisely which path they're hitting, but please let me know what you think (e.g. more concise, consistent, and descriptive alternatives) 🎉
Also, if you think there's any subtleties that are hard to succinctly capture in the test names, may you please add short docstring that describes the various parameterizations or any assumptions that might be baked into the test?

Hi Josh (@wheevu), Wanda and I have talked through the comments, and Wanda is just going to make a few small changes documented in the comments, to expedite completion of this PR. I think we should leave out the docstring requirement for now because it's not on any of the other tests, and in these tests we have a good idea of what is being tested from the good method names being used. There were some very cool tests in here, Josh (also noted in the comments, some of which you'll have to dig into because the conversations are resolved). Thank you for taking on this enhancement (and the issues in your other PRs).

Thanks, John and Wanda! I'll take some time to read through all the comments properly. There's a lot for me to learn from the discussion. I really appreciate you both working through this and taking care of the final changes. Glad the tests were useful! 😄

No worries at all; it's my (and our) pleasure!! It's wonderful to have help coming on on these issues, and your null byte detection and handling + unicode filename support are excellent finds! I'll go through and make the remainder of these small changes, then will get this merged in, so no need to do anything on your end <3

I'll keep ya posted if we need anything on the subsequent PRs 🎉 Thank you again, Josh (@wheevu )

[wbkboyer]

Ready to rock! Thank you again, @wheevu 🎉