Skip to content

[TASK] Deprecate __toString() #1006

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

Merged
merged 1 commit into from
Feb 27, 2025
Merged

[TASK] Deprecate __toString() #1006

merged 1 commit into from
Feb 27, 2025

Conversation

oliverklee
Copy link
Collaborator

Part of #998

@coveralls
Copy link

coveralls commented Feb 26, 2025

Coverage Status

coverage: 54.784%. remained the same
when pulling 595da81 on deprecate/to-string
into 215c199 on main.

Copy link
Collaborator

@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.

The deprecation notices are all fine, but the moving of a test would seem to belong in a separate PR (if required at all).

/**
* @test
*/
public function toStringRendersCommentEnclosedInCommentDelimiters(): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this method being moved here to test something that's deprecated? Shouldn't the introduction of the test class be a separate PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm moving this one test from Tests/Functional/ to Tests/FunctionalDeprecated/, where tests for deprecated functionality should be placed. (This will allow us to have different PHPUnit configuration files later on, allowing calls to deprecated functionality in Tests/FunctionalDeprecated/, but not in Tests/Functional/.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies. I overlooked the location of where this was being moved to, and instead assumed (without looking) it was a new test class being created in the tests/Unit directory.

That said, I'm now wondering why the CommentTest class is in Functional rather than Unit, given that it appears to be a unit test for the Comment class - but that's beyond the scope of this PR.

Copy link
Collaborator

@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.

My mistake in previous review. Though I do have a further question in the comments.

/**
* @test
*/
public function toStringRendersCommentEnclosedInCommentDelimiters(): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies. I overlooked the location of where this was being moved to, and instead assumed (without looking) it was a new test class being created in the tests/Unit directory.

That said, I'm now wondering why the CommentTest class is in Functional rather than Unit, given that it appears to be a unit test for the Comment class - but that's beyond the scope of this PR.

@JakeQZ JakeQZ merged commit 0bfa3f3 into main Feb 27, 2025
21 checks passed
@JakeQZ JakeQZ deleted the deprecate/to-string branch February 27, 2025 09:09
@oliverklee
Copy link
Collaborator Author

That said, I'm now wondering why the CommentTest class is in Functional rather than Unit, given that it appears to be a unit test for the Comment class - but that's beyond the scope of this PR.

I see the tests for render (and for __toString) as we're testing the tested class in conjunction with OutputFormat and OutputFormatter instead of the tested class in isolation.

oliverklee added a commit that referenced this pull request Feb 27, 2025
Part of #998

This is the V8.x backport of #1006.
@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 27, 2025

That said, I'm now wondering why the CommentTest class is in Functional rather than Unit, given that it appears to be a unit test for the Comment class - but that's beyond the scope of this PR.

I see the tests for render (and for __toString) as we're testing the tested class in conjunction with OutputFormat and OutputFormatter instead of the tested class in isolation.

I (originally) saw the functional tests department as mainly a place for testing the various CSS conent that is in tests/fixtures that does not test any class directly. But yes, I see in CommentTest we are testing the behaviour in conjunction with OutputFormat, which cannot be done in isolation.

oliverklee added a commit that referenced this pull request Feb 27, 2025
Part of #998

This is the V8.x backport of #1006.
oliverklee added a commit that referenced this pull request Mar 1, 2025
Part of #998

This is the V8.x backport of #1006.
JakeQZ pushed a commit that referenced this pull request Mar 3, 2025
Part of #998

This is the V8.x backport of #1006.
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