[rafugafu]

Replaced the custom terminal with an advanced PTY terminal (a PyQt port of PyTkTerm) and updated the terminal tests.
The terminal restarts on a key press after the process ends.
The terminal updates for the editor font but cannot change the editor font family (terminals should remain monospace font).

[AlexHettle]

Hey there Rafugafu,

Thanks so much for putting this together! I really appreciate the time and effort you’ve put into improving MeadowPy’s terminal. I think this will be a big step forward compared to the terminal we previously had.

The main thing holding me back from approving and merging right now is testing. CI shows all previous existing tests passing, which is encouraging, but overall coverage with these changes is 87.23%, below the project’s 95% requirement. The three POSIX-specific terminal tests are also skipped on our Windows runner. I'm pretty sure POSIX is really only for Linux/MacOS, not Windows, so this is most likely why the new tests are getting skipped and possibly why there seems to be a new dip in coverage since those new tests are being ignored while the new functionality isn't.

Could you add/fix tests for the new terminal behavior, particularly parsing/input handling and Windows process startup, shutdown, and failure recovery? I’d like to get CI passing without lowering the coverage requirement.
Thanks again for the contribution! I’m looking forward to revisiting approval once these testing gaps are addressed.

[rafugafu]

Hi AlexHettle,

This happens because the POSIX tests are correctly skipped on Windows, reducing the coverage. There are two ways this can be fixed without lowering fail_under:

1. Write many more tests for terminal.py on its escape sequence parsing, etc. There can't be much failure recovery, since this is a real terminal emulator which runs the real powershell on your system and just parses its output, and does not try to simulate a shell itself. So it will not usually throw a Python error. The only problem which can happen is a visual glitch caused by a bug in parsing, after which recovery is not easily possible anyway in advanced terminal applications because even one character off can cause the whole UI to show wrong.

2. Omit terminal.py from coverage, since it is just a packaged copy of pyqtterm (https://github.com/rafugafu/pyqtterm), and maintained separately (you wouldn't have tests for an external module). I would also rename it.

I would go with the second option for now, since the terminal part is not really part of or maintained by MeadowPy yet. If there are bugs in it, they should go to pyqtterm not MeadowPy.

By the way, MeadowPy already works almost perfectly fine on Linux, which is why I can do this. So getting Linux support should not be too hard.

[rafugafu]

I have done that.

[AlexHettle]

Thanks for explaining, and again for all the work you’ve put into this! I understand the terminal code comes from your PyQtTerm project, but I’m hesitant to exclude the file from coverage on that basis alone.
My concern is also about long-term maintenance. With the implementation included directly in MeadowPy, fixes made in PyQtTerm won’t automatically reach our copy, and any MeadowPy-specific changes could make future updates harder to bring over. I also didn't see any testing suite of your own on your repo, which make my concern about untested code being added into MeadowPy still stand.

I’d be more comfortable with either of these approaches:

1. Treat it as a MeadowPy-specific implementation, with tests in this repo covering its behavior and the changes we maintain

2. Make PyQtTerm an installable, versioned dependency that MeadowPy imports, with its own test suite maintained on your repo. MeadowPy would still need integration tests, but updates could be handled through reviewed dependency upgrades rather than manually synchronizing copied code.

It's really important to have high coverage rates in a codebase and is a common business standard, it's a pivotal practice that helps prevent code from breaking with new changes over time especially when a project gets larger and more complex. Excluding the file might address the coverage number, but it doesn’t resolve that underlying concern.
Would either approach work for you? I appreciate the contribution and would like to get this into a form we can confidently maintain before approving and merging.

[rafugafu]

Ok, I will make more tests for the terminal.

[rafugafu]

I added many terminal tests in meadowpy, is this fine? Also pyqtterm is not at all untested it is very well tested by myself in real terminal applications, with scripts, and in already-made tests like vttest, but it is correct that there are no tests on the git repository.

[AlexHettle]

Hey there,

Thanks for writing those tests! I should clarify, I have full confidence that you tested your code extensively, but my concern was the fact that there was no public facing evidence of testing, which is an important aspect of building trust and reliability in open-source dev.

I see now that the coverage threshold has been met again which is great, but it looks like one of your introduced tests for the terminal is failing:

From the quick look I took this seems like a simple fix and everything else regarding the PR looks good so if you can address this last little issue I'd be happy to merge. Thanks again!

[rafugafu]

Done, it was a simple fix. I saw all tests and the workflow pass.

[AlexHettle]

Hey there,

Thanks for that last fix, and for this work overall. Merging now, in the future if I find issues with this new functionality I'll write it up and assign you to it. Thanks again!