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

Unify Printer, Logger and Console classes #2124

Merged
merged 10 commits into from
Jan 22, 2025

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Jan 20, 2025

Following the discussion in #2115 - unifies the Printer, Logger and Console classes into a single Console class.

This allows a single representation of "interaction with the user", capturing both input and output, which also writes to a log file as required. It also cleans up some odd API usage (such as needing to invoke input.wait_bar(), and the __call__ interface on input (it can now be replaced with an input() method.

This PR builds on #2115 to avoid collisions with that work. You could either review, approve and merge this PR by itself; or review and merge #2115, then rebase, review and merge this one. UPDATE: PR has now been rebased onto main.

It is, unfortunately, a very large PR... but most of the changes are renames.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742 freakboy3742 marked this pull request as ready for review January 20, 2025 11:36
@freakboy3742
Copy link
Member Author

Apologies in advance for this monster :-)

Copy link
Member

@rmartin16 rmartin16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important bits in console.py look good. Since we're delineating the groups of related methods with "hashtag boxes", do you want to add one above textwrap() for "user input and output formatting"....or something like that :)?

Otherwise, I'm hoping to get through this tomorrow.

Copy link
Member

@rmartin16 rmartin16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments while I'm making my way.

changes/2114.feature.rst Outdated Show resolved Hide resolved
src/briefcase/console.py Show resolved Hide resolved
src/briefcase/console.py Outdated Show resolved Hide resolved
@rmartin16
Copy link
Member

Looks like tests/console/Console/test_selection.py needs to be updated for console._selection() but I think I'm good with this. I didn't review all of the changes in tests like I did src....but if anything isn't causing the tests to fail, we'll have to fix it later :)

@freakboy3742
Copy link
Member Author

Looks like tests/console/Console/test_selection.py needs to be updated for console._selection() but I think I'm good with this. I didn't review all of the changes in tests like I did src....but if anything isn't causing the tests to fail, we'll have to fix it later :)

/me grumbles... I thought I ran those tests...

@freakboy3742
Copy link
Member Author

Looks like tests/console/Console/test_selection.py needs to be updated for console._selection() but I think I'm good with this. I didn't review all of the changes in tests like I did src....but if anything isn't causing the tests to fail, we'll have to fix it later :)

/me grumbles... I thought I ran those tests...

Dammit... I did... and then didn't stage the change in git. 🤦

@freakboy3742 freakboy3742 merged commit 6fb23c1 into beeware:main Jan 22, 2025
57 checks passed
@freakboy3742 freakboy3742 deleted the consolidate-console branch January 22, 2025 03:24
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