Add AbstractPort
Description of issue:
Rename the existing class from Port to AbstractPort and add a Port interface class that inherits from ABC. Can use the existing Port tests to conform that the functionality is still working.
Linked Requirements:( Y )
Potential Issues:None
Blockers:( #X )
Issue Requirements:- Class structure should conform with the Class Diagram in the Architects Documents.
Should be easy to implement.
Activity
mentioned in issue #49 (closed)
assigned to @anglo547
created branch
48-add-abstractportto address this issueWorking on this issue I discovered some differnces between the class diagram and the current implementation, should these functions that are currently implemented be removed:
- is_connected_to_signal
- signal_count
The reasons to keep these functions are to reduce code duplication, but the reason to remove them would be to reduce the amount of available functions to the user, what should be done here?
The function disconnect_signal(signal) from the class diagram is implemented as disconnect_signal_by_ref(signal), currently disconnect_signal(index) takes an index of the signal to be removed. Should the class diagram or the implementation be changed?
Also, should the property getters for that returns a list of connected Ports / Signals be called "signals" / "connected_ports" as they currently are? Or should "signals" be changed to "connected_signals" as in the class diagram?
Also, the functions named "connect" and "disconnect" in the class diagram are implemented as "connect_port" and "disconnect_port". I prefer the current implementation since there is also a function called "connect_signal", so if we have a function called "connect" and then one called "connect_signal" it becomes unclear until the user checks the documentation which function is the correct one. Meanwhile "connect_port" and "connect_signal" makes it clear which type that should be used for which function. What should be done here?
Asking @ivaha717 but @jacwa448 's opinion would also be appreciated.
The method is_connected_to_signal should definitely be removed. It bloats the API since its functionality is not needed often, and when it's needed, it's just as clear to write "if s1 in p1.connected_signals():" as it is to write "if p1.is_connected_to_signal(s1):". In addition, the name does not make sense, since a port is not conceptually "connected" to a signal. Rather, ports are conceptually connected to other ports, with signals just being the things that connect them.
The implementation of disconnect_signal should be changed to reflect the API of the class diagram. I realized that disconnecting by index is seldom useful since it's more likely that you have a reference to the signal you want to disconnect than it is that you remember which order the signals were connected in. Simply remove the version that takes an index and rename the _disconnect_signal_by_ref method to disconnect_signal.
The unique naming for the connect/disconnect methods that take a port stems from the fact that connect returns a signal, which makes the function signature fundamentally different from connect_signal. Therefore I think naming them similarly to connect_signal would only be dishonest. The connect method that takes a port is also expected to be used more frequently, which makes a short name that is easy to remember more appropriate. To further disambiguate the two, we could consider renaming connect_signal to add_signal and disconnect_signal to remove_signal, as well as renaming disconnect_all to clear, connected_signals to signals, and connection_count to signal_count.
In any case, the class diagram should take precedence over whatever the code happens to say right now. If the API in the class diagram is bad, we should discuss changing the class diagram rather than making deviations in the code.
The point in having the discussion here is to make sure that we change the class diagram if needed. Since the code was developed in parallel to the class diagram there will be differences, and it is certainly true that the class diagram doesn't have everything figured out.
One reason to keep the "is_connected_to_signal(s1)" function is that it abstracts away how signals actually are saved in the port, perhaps how this is implemented will be changed in the future (like perhaps adding a set for allowing O(1) lookup time), and abstracting away the implementation allows for such changes. Also, checking if a port is connected to a signal could be different from an InputPort and an OutputPort as they save them differently and having a function in their parent interface allows for optimal identical treatment between them. However I do agree that since this function isn't used in so many places and that we don't want to clutter the amount of functions then it could be removed.
Alright I agree that naming "connect_port" just "connect" would be good if we also renamed "connect_signal" to "add_signal". The proposed changes from "disconnect_signal" to "remove_signal", "disconnect_all" to "clear", "connected_signals" to "signals", and "connection_count" to "signal_count" sound reasonable and inline with how the code looks currently.
Edited by Angus Lothian-
I agree with the general idea of abstracting away how the signals are stored. The problem is that "p1.is_connected_to_signal(s1)" does not abstract away anything of importance that isn't already abstracted away in "s1 in p1.signals()". If O(1) lookup was important, we could change p1.signals() to return a set, and all the code that uses it would still work. (However, given how few signals will typically be connected to the same port, using a set to store them might actually be a pessimization in the common case.) Furthermore, section 5.3 in the architecture description states that we should generally prefer simplicity over insignificant optimizations in the Python interface. With that in mind, I believe that if anything, the function "is_connected_to_signal" should be removed simply because it adds an unnecessary obfuscation to the general idea of checking whether an item is part of a collection. Python's "in" operator already covers that in a way that users who have worked in Python before are used to and can understand at a glance without having to read the documentation.
Agreed.
By the way, I noticed that the connect terminology is used in "signal.connect_source(port)" and "signal.connect_destination(port)", so this means that we can "connect" a signal to a port. But then we have renamed "port.connect_signal(signal)" to "port.add_signal(signal)" because it would not be correct terminology to "connect" a port to a signal, is this contradicting for the user?
If you look at the current implementation then the "connect_source/destination" functions also connect the port to the signal if it isn't already connected. That way if the user calls "s1.connect_source(p1)" then s1 is "connected" to p1, and p1 is also "connected" to s1. So I think the name "connect_source", or something similiar, better describes what is actually done in the function, since if the name is just "set_source" then the user might just think that it's a normal setter.
Add abstract port and some changes required to fit the class diagram. Also change port id to port index which was suppose to be done in #49 (closed) but was easy to do in combination with the changes for this issue.
Moved AbstractGraphComponent to the graph_componet module to fit with the class diagram. Was also attempting to move the AbstractOperation to the operation module but it caused a cyclic import with utilities so the changes where reversed, however this problem has to be fixed sometime. Perhaps the utilities function could be moved to the operation module as well as it is only used there currently.
Also changed the functions to the names agreed to in the previous discussion for Port and Signal.
Add clear function to Port. Will need a test that it works as intended.
mentioned in merge request !11 (merged)
Fix som old usage of port index and rename input port "_signal" to "_source_signal" and output port "_signals" to "_destination_signals" to conform to the class diagram.
Create merge request for issue in !11 (merged)
Add estimate from #49 (closed) (1h) which is also implemented in this issue in !11 (merged)
Edited by Angus Lothianadded Needs review label and removed 1 deleted label
Closed in !11 (merged)
removed Needs review label
changed milestone to %Sprint 2
