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

Allows setting/resetting entity.rgb to None #1116

Closed
wants to merge 1 commit into from

Conversation

kkkkkom
Copy link

@kkkkkom kkkkkom commented Jun 26, 2024

Hi, @mozman , could you please review this minor merge request?

I think we should allow entity.rgb to be set/reset to None.

Many entity rgbs are initially None by default, and this change will allow users/developers to temporarily set rgb and reset it back without causing an error.

pytest has passed clean locally.

Copy link
Owner

@mozman mozman left a comment

Choose a reason for hiding this comment

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

This is not the correct way.

If you rally need this feature implement the @rgb.deleter method and check if the attrib true_color exists otherwise a DXFAttributeError will be raised.

@kkkkkom
Copy link
Author

kkkkkom commented Jun 26, 2024

This is not the correct way.

If you rally need this feature implement the @rgb.deleter method and check if the attrib true_color exists otherwise a DXFAttributeError will be raised.

Thanks for reviewing, I'll update accordingly

@kkkkkom
Copy link
Author

kkkkkom commented Jun 26, 2024

Hi, @mozman , could you please review again?

Set RGB true color as (r, g , b) tuple e.g. (12, 34, 56).
Also allows setting/resetting rgb to None.
"""
if rgb is None:
Copy link
Owner

Choose a reason for hiding this comment

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

This case should raise a TypeError, None is not a valid input or revert everything because the previous version raised the TypeError automatically.

mozman added a commit that referenced this pull request Jun 27, 2024
- implement DXFGraphic.rgb deleter method.
- add tests for DXFGraphic.rgb
- update docs and changelog
@mozman
Copy link
Owner

mozman commented Jun 27, 2024

@kkkkkom thanks for the idea, to speed up the process I added all the necessary changes myself

@mozman mozman closed this Jun 27, 2024
@kkkkkom
Copy link
Author

kkkkkom commented Jun 27, 2024

@kkkkkom thanks for the idea, to speed up the process I added all the necessary changes myself

Thanks!

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