Skip to content

Conversation

@munechika-koyo
Copy link
Member

Key changes:

  • Rename TargettedPixelGroup to TargetedPixelGroup and add deprecation warnings.

  • Remove unnecessary blank lines for improved code formatting.

Copy link
Member Author

@munechika-koyo munechika-koyo left a comment

Choose a reason for hiding this comment

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

Should wait for #486 merged to run CI correctly.

@skuba31
Copy link
Contributor

skuba31 commented Oct 17, 2025

I have only one minor point regarding the deprecation warning. Should an explicit version number be added to the message? And if so, should it be 2.0 or a different one?

@munechika-koyo
Copy link
Member Author

@skuba31 Thanks for your feedback!
I specified the version number when we will remove the deprecated API.

@jacklovell
Copy link
Member

2.0 is the appropriate place to remove deprecations, agreed. Have you checked if there are any public attributes or methods in classes which use the new Targeted spelling? For subclasses of Raysect Targeted* classes these may be present, in which case you should also add aliased methods in the compatibility class.

I wouldn't want to go too far down this rabbit hole though. We can do what we can to provide backwards compatibility, but users will find stuff breaks anyway if they use any of the Raysect classes directly so it'll never be a completely clean switch.

@munechika-koyo munechika-koyo marked this pull request as ready for review November 13, 2025 12:35
Copy link
Member

@Mateasek Mateasek left a comment

Choose a reason for hiding this comment

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

Hi @munechika-koyo , thanks for taking care of the renaming and backwards compatibility.

Could you please add the TargetedPixelGroup to the documentation? I know it is almost a duplicate of the TargettedPixelGroup docstring, but since you added the deprecation warning, I think it should be visible from the documentation that there is a replacement class available.

@Mateasek
Copy link
Member

Mateasek commented Nov 26, 2025

Also, could you please make an issue about removing the deprecated TargettedPixelGroup when we move to Cherab 2.0 version? I created a new milestone for the version 2.0, please link it with the issue so we don't forget all the deprecated code to be removed.

@Mateasek
Copy link
Member

Thank you for adding the record into documentation and for creating the issue.

Also, thank you very much for the PR @munechika-koyo, I'm merging this into development.

@Mateasek Mateasek merged commit 0c98d56 into cherab:development Nov 27, 2025
6 checks passed
@munechika-koyo munechika-koyo deleted the feature/API-change-targetedpixel branch November 27, 2025 12:59
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.

4 participants