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

Extract print methods to seperate classes #854

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

p8
Copy link
Member

@p8 p8 commented Aug 9, 2023

The printing methods are already pretty large, making it difficult to add extra functionality. Extracting them to seperate classes allows refactoring them for easier maintainability.

A lot of the functionality of calculating the terminal width can be extracted to a separate object as well.

@p8 p8 force-pushed the refactor/extract-printers branch 4 times, most recently from d1cf241 to 5f4f214 Compare August 9, 2023 11:50
@@ -0,0 +1,13 @@
class Thor
module Shell
class Printer
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this class? Can't we just merge it with ColumnPrinter?

Copy link
Member Author

Choose a reason for hiding this comment

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

TablePrinter and WrappedPrinter inherit from Printer as well.
All three subclasses implement the print method in their own way.

Copy link
Member

Choose a reason for hiding this comment

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

Can't we change them to inherit from ColumnPrinter?

Copy link
Member

Choose a reason for hiding this comment

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

I'm asking this because Printer doesn't have the same interface as their subclasses so it is breaking the Liskov substitution principle, and I see no reason to break that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

The printing methods are already pretty large, making it difficult to
add extra functionality. Extracting them to seperate classes allows
refactoring them for easier maintainability.

A lot of the functionality of calculating the terminal width can be
extracted to a separate object as well.
@p8 p8 force-pushed the refactor/extract-printers branch from 5f4f214 to 580234a Compare August 22, 2023 07:38
@rafaelfranca rafaelfranca merged commit 9c9ab52 into rails:main Aug 22, 2023
7 checks passed
simon04 referenced this pull request in freeCodeCamp/devdocs Jan 5, 2024
The newest Thor also contains a Terminal module, causing "uninitialized constant Thor::Shell::Terminal::Table (NameError)"
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