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

[TASK] Deprecate method forwarding OutputFormat to OutputFormatter #894

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oliverklee
Copy link
Contributor

Instead, the corresponding method on the formatter should be called directly.

This increases type safety and helps static code analysis. Also, it makes the code easier to understand.

@coveralls
Copy link

coveralls commented Feb 9, 2025

Coverage Status

coverage: 51.335%. remained the same
when pulling b4d7c71 on task/deprecate-forwarding
into d71ff28 on main.

Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

I think we need to provide chaining methods before we can do this.

If you edit __call() to throw an exception in all cases, there are many test failures.

The chaining methods required are clearly-defined by OutputFormatter. There aren't that many.

I think this would be easier and more preferable than rewriting the tests and changing the API - bear in mind that all users would have to expend similar effort rewriting their code as we would rewriting the tests, if we were to change the API.

@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 10, 2025

The chaining methods required are clearly-defined by OutputFormatter.

When they are all in place, __call() can simply be removed.

@oliverklee
Copy link
Contributor Author

I've updated the calls in #898.

@oliverklee oliverklee requested a review from JakeQZ February 10, 2025 09:17
@oliverklee
Copy link
Contributor Author

I think this would be easier and more preferable than rewriting the tests and changing the API - bear in mind that all users would have to expend similar effort rewriting their code as we would rewriting the tests, if we were to change the API.

I consider the OutputFormatter to be internal to our library (#896), and hence this change should have no effect on users of our library.

@oliverklee oliverklee force-pushed the task/deprecate-forwarding branch from 7d695b6 to ff42244 Compare February 10, 2025 09:23
@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 10, 2025

I think this would be easier and more preferable than rewriting the tests and changing the API - bear in mind that all users would have to expend similar effort rewriting their code as we would rewriting the tests, if we were to change the API.

I consider the OutputFormatter to be internal to our library (#896), and hence this change should have no effect on users of our library.

I agree, I think. Which is all the more reason to add these methods to OutputFormat so that users don't need to access the underlying OutputFormatter, whether that be by obtaining the object or using __cal().

@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 10, 2025

I think this would be easier and more preferable than rewriting the tests and changing the API - bear in mind that all users would have to expend similar effort rewriting their code as we would rewriting the tests, if we were to change the API.

I consider the OutputFormatter to be internal to our library (#896), and hence this change should have no effect on users of our library.

I agree, I think. Which is all the more reason to add these methods to OutputFormat so that users don't need to access the underlying OutputFormatter, whether that be by obtaining the object or using __cal().

... unless these methods are all meant for internal use. Will need to check...

@oliverklee oliverklee force-pushed the task/deprecate-forwarding branch from ff42244 to 898fac5 Compare February 10, 2025 10:57
@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 11, 2025

I've updated the calls in #898.

I think that makes it clear. These are all (only) used by the internal render methods, as a convenience to avoid the extra getFormatter call, and are not intended to be part of the public API. @sabberworm, is that correct?

@oliverklee oliverklee force-pushed the task/deprecate-forwarding branch 6 times, most recently from 21d38b2 to 04a3c1a Compare February 15, 2025 09:40
@oliverklee
Copy link
Contributor Author

Ping @sabberworm - we need your input here in order to proceed.

@oliverklee oliverklee force-pushed the task/deprecate-forwarding branch 6 times, most recently from a0b2253 to 8e900ef Compare February 16, 2025 21:20
Instead, the corresponding method on the formatter should be called
directly.

This increases type safety and helps static code analysis. Also, it
makes the code easier to understand.
@oliverklee oliverklee force-pushed the task/deprecate-forwarding branch from 8e900ef to b4d7c71 Compare February 18, 2025 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants