Lots of Errors, Warnings and such within LXMF.py

Question

Lots of Errors, Warnings and such within LXMF.py

Closed this issue 2 years ago · 5 comments

Hi Mark,
on my investigation regarding how I can get a notifcation from LXMF that a message has arrived at a destination, I took a deep look into the LXMF.py code.
I pulled the file into my PyCharm IDE and got quite a lot Warnings and code hints of all kind.
Some of them may be quite critical...

First: Line 233

            # If no desired delivery method has been defined,
            # one will be chosen according to these rules:
            if self.desired_method == None:
                self.desired_method == LXMessage.DIRECT
            # TODO: Expand rules to something more intelligent

Guess here is the reason why leaving desired_method unset while calling LXMessage() does not work.
Instead of:
self.desired_method == LXMessage.DIRECT
I think it should state:
self.desired_method = LXMessage.DIRECT

Second: Line 314

    def determine_transport_encryption(self):
        if self.method == LXMessage.OPPORTUNISTIC:
            if self.__destination.type == RNS.Destination.SINGLE:
                self.transport_encrypted = True
                self.transport_encryption = LXMessage.ENCRYPTION_DESCRIPTION_EC
            elif destination_type == RNS.Destination.GROUP:
                self.transport_encrypted = True
                self.transport_encryption = LXMessage.ENCRYPTION_DESCRIPTION_AES
            else:
                self.transport_encrypted = False
                self.transport_encryption = LXMessage.ENCRYPTION_DESCRIPTION_UNENCRYPTED

Here the line:
elif destination_type == RNS.Destination.GROUP:
May probably should state ...
elif self.__destination_type == RNS.Destination.GROUP:
... since it is used later this way.
But in this case the object has no such member declared to judge on that assumption.
In general there are lots of members that are not declared explicitely, making reading the code even more cumbersome.

Third: Line 1269

        except Exception as e:
            RNS.log("Could not enable propagation node. The contained exception was: " + str(e), RNS.LOG_ERROR)
            raise e
            RNS.panic()

Here RNS.panic() is just dead code, that can not be reached,
And to use raise e is discouraged, because simply raise ususally puts the line into the log that contains the error and not the line of the raise statement as with raise e.

So probably the above snipped should be:

        except Exception as e:
            RNS.log("Could not enable propagation node. The contained exception was: " + str(e), RNS.LOG_ERROR)
            raise

Since I have no clue if the RNS.panic() was placed there with purpose, I cant really correct that one properly.

Forth: Line 1637

        for peer in culled_peers:
            RNS.log("Removing peer " + RNS.prettyhexrep(peer) + " due to excessive unreachability", RNS.LOG_WARNING)
            try:
                # TODO: Check this
                if peer_id in self.peers:
                    self.peers.pop(peer_id)
            except Exception as e:
                RNS.log(
                    "Error while removing peer " + RNS.prettyhexrep(peer_id) + ". The contained exception was: " + str(
                        e), RNS.LOG_ERROR)

Since PyCharm told me that peer_id within
if peer_id in self.peers:
migth be accessd prior its initialized, the snippet might be changed into:

        for peer_id in culled_peers:
            RNS.log("Removing peer " + RNS.prettyhexrep(peer_id) + " due to excessive unreachability", RNS.LOG_WARNING)
            try:
                # TODO: Check this
                if peer_id in self.peers:
                    self.peers.pop(peer_id)
            except Exception as e:
                RNS.log(
                    "Error while removing peer " + RNS.prettyhexrep(peer_id) + ". The contained exception was: " + str(
                        e), RNS.LOG_ERROR)

Same as before, I'm not sure if thats what you intendet, especially because there is this ToDo for checking this there.
Thats where I stopped analysing all the hints and warnigs indicated by PyCharm.

And there is still lots more of other stuff, that may need some attention too:

Unresoved attribute references
Variable references before assignments
Deprecated setDaemon() usage
And dozens of other warnings and PEP 8: E711 code warnings

Is that file the most recent one?
If so why not putting some love into cleaning up that awsome piece of network code in a way, that it does not have that many warnigs and improvement hints any longer?

Cheers
Stephan

HarlekinSimplex commented 2 years ago

Yes. :-)

Answer 1 · 2022-04-13T09:34:49.000Z

Hi Stephan

Instead of:
self.desired_method == LXMessage.DIRECT
I think it should state:
self.desired_method = LXMessage.DIRECT

Thanks, that is indeed a typo. Fixed in 505176b.

Here the line:
elif destination_type == RNS.Destination.GROUP:
May probably should state ...
elif self.__destination_type == RNS.Destination.GROUP:
... since it is used later this way.
But in this case the object has no such member declared to judge on that assumption.

Correct again. Fixed in 2d18285. This is actually a remnant of skeleton code, that was not yet updated, since group destinations in LXMF is not yet implemented. But I'll get back to that.

Third: Line 1269
Here RNS.panic() is just dead code, that can not be reached, And to use raise e is discouraged, because simply raise ususally puts the line into the log that contains the error and not the line of the raise statement as with raise e.

No, RNS.panic() gracefully terminates the stack and the currently attached program, which was the intention here. Whether that is a bit overkill could be argued. It made sense when the decision was made, but maybe we should just raise the exception instead.

Stylistically I think it makes more sense to explicitly raise e. Semantically it makes no difference, and is consistent with the style of everything else.

Since PyCharm told me that peer_id within
if peer_id in self.peers:
migth be accessd prior its initialized, the snippet might be changed

Yes, that is a typo, it should indeed be peer_id. Fixed in ed4effd.

Thanks a lot for spotting and taking the time to report these!

Answer 2 · 2022-04-13T09:45:09.000Z

Next, regarding the stylistic proposals from PyCharm: I am not generally in favor of blindly following code improvement proposals from an IDE. It is a great tool in some cases, but really, all such proposals from a completely automated and "dumb" system needs to be taken with a grain of salt, and actually be completely understood in the context of the code.

When LMXF+RNS reach a point where few functional changes are being committed, and we have a more or less fully covering test suite, I think it is a meaningful time to look at stylistic improvements. Until then I am going to block efforts towards shuffling whitespace around, and dancing to the tune of automated IDE warnings.

Answer 3 · 2022-04-13T14:09:17.000Z

A lot of those warnings are about using 'if x is not None' insteadt of 'if x != None' or 'if x not in y' instead of 'if not x in y' and like things.
From my point of view this is more about avoiding error prone usage of the language than stylistic shuffling of white space.
Tough a lot of people out there may put all those things including the white space stuff into the same bucket called overall code quality.
Experience shows, that if not put in rigth from the beginning, it will never be put in later.

Eventually I got sick of always seeing and ignoring those warnings and started to do so as suggested.
And guess what! Having the '!Problems' tab empty after any change I introduced gave me a lot of psychological safeness , good feelings and assurance that there is no easy to avoid issue left.
By now I seek to harvest those rewardings from any IDE, regardless which language Im actually use.
At the end of the day it costs no extra time to do so.
It's just a change of mindset one need to adopt to that pays off with improved overall code quality and readability.

And why not dancing to the tune of automated IDE warnings? They are nothing else than resourcefully implemented hints for us developers to avoide costly learnings we are basically not aware of today, but lots of people have already suffered from in the past though.
Especially if it is just a matter of style and does not come with any relevant additional work load except slightly modifing personal habbits..

I'm looking forward to see the LXMF code beeing completed with the group destination management and code quality cleanup.
Maybe you can find some additional time too, to comment the code in a way that a curious developer like me has a chance to understand what that code is all about, without the need to fully analyse and understand each and every line and statement in detail.
Code can not replace documention. And yes, putting in lots of usefull code comments is another aspect of code quality, I have learned the hard way on my journey throughout four and a half decades of codeing with lots of different languages.

Cheers
Stephan

Answer 4 · 2022-04-13T15:59:31.000Z

Don't get me wrong, I don't dismiss it as such, it is just not the time for that right now, and if I start focusing on that, there's no point in not doing it everywhere, which will be a lot of time spent on something that is not the highest priority right now. There's a time and place for everything :)