-
Notifications
You must be signed in to change notification settings - Fork 2
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
Point name support #15
base: main
Are you sure you want to change the base?
Conversation
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.
Again, only minor comments. Thanks.
# Set default names for the rest | ||
for node in self.topology.nodes.values(): | ||
if node.name is None: | ||
node.name = node.uuid[-5:] |
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.
Assuming a risk of name clashes here. Also assuming that it wouldn't be a big problem, except for humans trying to debug problems.
@@ -93,14 +95,20 @@ def _get_other_uuid(_uuid, _edge): | |||
|
|||
def _get_next_edge(_last_node_uuid, _second_last_node): | |||
for _geo_edge in geo_edges: | |||
if _last_node_uuid in [_geo_edge.ID_GEO_Knoten_A.Wert, _geo_edge.ID_GEO_Knoten_B.Wert]: |
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.
For me, personally, "last" is a confusing identifier, maybe because it's a little false friend with German "letzte:r". It could mean "previous" but also "the last one we will see in the future". Take, for example the list 1, 2, 3, 4
. We iterate until 3
. Is "last" then 2
or always 4
? I sometimes use "previous", "youngest", or "before" instead.
IIRC, "last" is being used here and there in the code. Keeping consistency would outweigh the possible confusion explained above.
print( | ||
f"Warning: TOP_EDGE {top_kante_uuid} could not be completed, " | ||
f"since the chain of geo edges is broken after {last_node_uuid}. " | ||
f"This may cause errors later, since the topology is broken." |
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.
f-string w/o substitutions
if len(signal.Punkt_Objekt_TOP_Kante) != 1: # If other than 1, no real signal with lights | ||
if ( | ||
len(signal.Punkt_Objekt_TOP_Kante) != 1 | ||
): # If other than 1, no real signal with lights |
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 looks a bit over-formatted. Possible alternative:
if len(signal.Punkt_Objekt_TOP_Kante) != 1:
# If other than 1, no real signal with lights
…
continue | ||
if signal.Bezeichnung is None or signal.Bezeichnung.Bezeichnung_Aussenanlage is None: | ||
if ( |
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.
FYI, if we want to pylint this code at some point, PEP 8 prefers a different style of breaking long lines in if statements (ctrl+f if (
).
No description provided.