|
The code was mostly functional but hard to follow and not quite up to the best current practice.
What has been done:
Exception hierarchy cleanup. Now it is VyOSError > ConfigError > CommitError > ConfigLocked
VyOSError (base class) is raised when the user does something obviously wrong, like trying to
commit before entering conf mode. These are programming errors and not recoverable without
editing the code.
ConfigError other than CommitError is raised on set/delete command failures. These are usually
user input errors and cannot be recovered from without interacting with the user.
CommitErrors include commit failures due to user errors, and ConfigLocked condition.
ConfigLocked is raised when anoher commit is in progress. This is the only condition that can be
recovered from programatically (wait and try again).
This way people can write try/catch sequences that go from more specific down to VyOSError
and use different recovery strategies for different error classes.
These names are also more self-explanatory than MaintenanceError etc.
Eliminate the last instances of functions that return error messages as strings instead of raising
exceptions, and especially those that extract error messages from exceptions caught within and return them.
Exceptions are easy to recognize and handle programmatically, string messages are not. Writing a
try/catch statement is quick, knowing what the message is and using re.search every time is inefficient
for both the user and the computer.
Replace inefficient and functionally limited "foo in bar" substring checks with regex matching.
Add handling of timeout errors, which was entirely absent (prompt() returns false in those cases,
easy to check).
Replace the __status dict with individual instance variables. There are few of them, and they are
always checked individually, so a dict is unwarranted. I kept the status() method that now wraps
those attributes in a dict. I also made it protected (single _) as it's debug stuff and not something
end users would normally want to touch.
Give attributes descriptive names (logged_in, conf_mode, session_modified, session_saved).
All attributes are now boolean. It both makes it more efficient than comparing strings,
and easier to understand what values they can have. It would be very hard to deduce that "status"
can be "login" or "logout" (or None, but only before the user logins) and "save" can be "Yes" or None
without reading the whole thing, and even harder to remember.
Simplify the error handling. Eliminate the has_error being set to "Type1", "Type2" etc.. This is very
hard to follow especially when the mapping of those strings to actual error strings is partially in different file.
This is affectionately known as "spaghetti code". ;)
It's been replaced with simple conditionals that raise appropriate exceptions as they go.
Eliminate unnecessary attribute checking and setting. If the state machine model cuts off erroneous paths
early and sets the state properly, there is simply no way the conf_mode flag can be True while logged_in is False,
for instance. There is also no need to unset say session_modified in save() because there is no way it can be True
if save() got to that state without throwing an error. It's (now) easy to see, and, if needed, formally prove.
Always reuse the pxssh connection object created by __init__, never make a new one, as it's redundant.
Replace all occurences of type() with isinstance() so that it also works for all subclases of the type/class in question.
Eliminate inconsistent use of stuff like """ foo is "Yes" """, """ foo == None """ and so on. Boolean checks all should be
like """ if foo: something() """ or """ if not foo: """, None checks should be """ foo is None """,
and == should be reserved for values that are neither None nor boolean.
Pass user and password to the constructor as separate arguments for convenience, rather than have the user join them just
to split them back.
Eliminate the "basic_string" dict. Command prefixes like set and delete always stay the same anyway, and they are only used
in corresponding functions.
|
