-
Notifications
You must be signed in to change notification settings - Fork 229
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
[WIP] Implement single wire cable #405
base: master
Are you sure you want to change the base?
Conversation
a91961e
to
1c4fd68
Compare
@@ -271,6 +271,7 @@ class Cable: | |||
show_name: Optional[bool] = None | |||
show_wirecount: bool = True | |||
show_wirenumbers: Optional[bool] = None | |||
show_colorname: bool = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest a different name:
show_colorname: bool = True | |
show_wirecolor_text: bool = True |
Alternatively show_color_text
, but then someone might expect it to also affect showing the cable.color
(jacket color) text. Do you want that?
I suggest text
instead of name
because the text might be different text representations of the colors, depending on options.color_mode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we are on the same page here. In a single-core cable, the color of the wire and the jacket would be identical. Anyway thanks for mentioning this, because it needs to apply to multi-core cables as well.
IMO neither text
nor name
is needed. In order to keep it intuitive, I consider the following for alignment with the other show_
switches:
cables:
w:
color: BK # jacket color box, omit this to hide it all
show_color: plain # only hide the text from jacket color box
show_color: text # only show the text and hide the bgcolor
colors: [GY] # wire color (list), omit this for defaults
show_colors: plain # hide the color text from individual wires
show_colors: text # only show the text without wire coloring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This increases the code complexity slightly, but it seems easier to read, and hopefully is easier to understand as well.
I assume you'll also allow the values True
for showing everything and False
for showing nothing.
With None
as default value for show_colors
, you can set it in __post_init__()
default plain
for your single-core cable use case and True
otherwise.
PS: Maybe we in a future separate PR will consider collecting all show_*
switches like this:
show:
color: text
colors: True
wirecount: False
DRAFT implementation of