Another "Generator Expressions" improvements - #130
Conversation
The is no need to create the lists for the functions, iterator is still enough
According to: https://www.python.org/dev/peps/pep-0008/ Don't compare boolean values to True or False using ==. Yes: if greeting: No: if greeting == True: Worse: if greeting is True:
|
I have another possible improvement to the code. Accordding to: "https://docs.python.org/3.6/library/stdtypes.html#truth-value-testing", testing if list is empty can be done taking advantage of the fact, that: "Here are most of the built-in objects considered false: ........empty sequences and collections: '', (), [], {}, set(), range(0)". if len(some_list) > 0: is equvalent with (without calling len() function: if some_list: This change would require some "find and replace" across the whole code, but might be worth doing... Regards |
derv82
left a comment
derv82
left a comment
There was a problem hiding this comment.
The changes unrelated to wps seem fine to me, but see my comments on how wps == None is not the same as wps == False.
| elif self.wps == False: | ||
| elif not self.wps: | ||
| wps = Color.s('{O} no') | ||
| elif self.wps is None: |
There was a problem hiding this comment.
This if-statement would never run, because not None evaluates to True.
There was a problem hiding this comment.
Hmmmm, i have just made a quick test to resolve the situation and find possible solution - please try Yourself replacing the WPS value with "True/False/None":
WPS = None
if WPS:
print(True)
elif not WPS and WPS is not None:
print(False)
elif WPS is None:
print(None)
|
|
||
| # WPS | ||
| if target.wps != False: | ||
| if target.wps: |
There was a problem hiding this comment.
This is awkward... I recently changed Target.wps from a bool (binary) to bool + None (ternary).
True: Target is WPS and unlockedFalse: Target is not WPS.None: Target is WPS but locked
See
wifite2/wifite/tools/tshark.py
Lines 199 to 204 in 838ea43
I wanted to allow people to see Locked WPS targets, and still attack them if desired.
I should have added another field to model.target (such as target.wps_locked), but I was lazy & overloaded what wps meant.
| Color.pl('{+} {C}option:{W} including {G}5Ghz networks{W} in scans') | ||
|
|
||
| if args.show_bssids == True: | ||
| if args.show_bssidS: |
There was a problem hiding this comment.
This should be "if args.show_bssids:" with small "s" at the end - don't know why it come with capital letter from my code..
|
I will fix the WPS True/False/None thing (change to an Enum) to avoid confusion in the future. Unfortunately, this will create conflicts with this PR. |
To avoid confusion about wps = True/False/None. Came about because of #130
|
Can You manually merge the small changes in the code, or should I create the fresh & new PR ? |
To avoid confusion about wps = True/False/None. Came about because of derv82#130
To avoid confusion about wps = True/False/None. Came about because of derv82#130
2 participants
The is no need to create the lists for the functions, iterator is still enough