| Message ID | 1413486964-5183-1-git-send-email-patrickdepinguin@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Thomas, All, On 2014-10-16 21:15 +0200, Thomas De Schampheleire spake thusly: > From: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> > > check-requirements simply has to know if the results have to be sent, so > it can check on some extra requirements. The username and password are > irrelevant here. > This commit introduces a boolean variable do_send_results to hide these > details. > > Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> > --- > scripts/autobuild-run | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/scripts/autobuild-run b/scripts/autobuild-run > index 7497001..2ead1f2 100755 > --- a/scripts/autobuild-run > +++ b/scripts/autobuild-run > @@ -85,12 +85,12 @@ def check_version(): > print "ERROR: script version too old, please upgrade." > sys.exit(1) > > -def check_requirements(http_login, http_password): > +def check_requirements(do_send_results=False): > devnull = open(os.devnull, "w") > needed_progs = ["make", "git", "gcc", "timeout"] > missing_requirements = False > > - if http_login and http_password: > + if do_send_results: > needed_progs.append("curl") > > for prog in needed_progs: > @@ -553,8 +553,9 @@ if __name__ == '__main__': > check_version() > sysinfo = SystemInfo() > (ninstances, njobs, http_login, http_password, submitter) = config_get() > - check_requirements(http_login, http_password) > - if http_login is None or http_password is None: > + do_send_results = http_login and http_password I was told that we should no treat 'None' as 'False', or a non-empty string as 'True'. This should be something like: do_send_results = (not http_login is None) and (not http_password is None) Regards, Yann E. MORIN. > + check_requirements(do_send_results) > + if not do_send_results: > print "WARN: due to the lack of http login/password details, results will not be submitted" > print "WARN: tarballs of results will be kept locally only" > def sigterm_handler(signum, frame): > -- > 1.8.5.1 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi Yann, Thomas, All On Sat, Oct 18, 2014 at 12:20 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > Thomas, All, > > On 2014-10-16 21:15 +0200, Thomas De Schampheleire spake thusly: >> From: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> >> >> check-requirements simply has to know if the results have to be sent, so >> it can check on some extra requirements. The username and password are >> irrelevant here. >> This commit introduces a boolean variable do_send_results to hide these >> details. >> >> Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> >> --- >> scripts/autobuild-run | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/scripts/autobuild-run b/scripts/autobuild-run >> index 7497001..2ead1f2 100755 >> --- a/scripts/autobuild-run >> +++ b/scripts/autobuild-run >> @@ -85,12 +85,12 @@ def check_version(): >> print "ERROR: script version too old, please upgrade." >> sys.exit(1) >> >> -def check_requirements(http_login, http_password): >> +def check_requirements(do_send_results=False): >> devnull = open(os.devnull, "w") >> needed_progs = ["make", "git", "gcc", "timeout"] >> missing_requirements = False >> >> - if http_login and http_password: >> + if do_send_results: >> needed_progs.append("curl") >> >> for prog in needed_progs: >> @@ -553,8 +553,9 @@ if __name__ == '__main__': >> check_version() >> sysinfo = SystemInfo() >> (ninstances, njobs, http_login, http_password, submitter) = config_get() >> - check_requirements(http_login, http_password) >> - if http_login is None or http_password is None: >> + do_send_results = http_login and http_password > > I was told that we should no treat 'None' as 'False', or a non-empty > string as 'True'. This should be something like:' You can treat 'None' as 'False' and also a non empty string will value a logical 'True'. In a python interpreter, do bool(None) and bool('blabla'), you'll respectively get 'False' and 'True'. I hope I am not the one you told you that in the first place... > > do_send_results = (not http_login is None) and (not http_password is None) > > Regards, > Yann E. MORIN. > >> + check_requirements(do_send_results) >> + if not do_send_results: >> print "WARN: due to the lack of http login/password details, results will not be submitted" >> print "WARN: tarballs of results will be kept locally only" >> def sigterm_handler(signum, frame): >> -- >> 1.8.5.1 >> >> _______________________________________________ >> buildroot mailing list >> buildroot@busybox.net >> http://lists.busybox.net/mailman/listinfo/buildroot > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------' > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Maxime, All, On 2014-10-18 12:15 +0200, Maxime Hadjinlian spake thusly: > On Sat, Oct 18, 2014 at 12:20 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Thomas, All, > > > > On 2014-10-16 21:15 +0200, Thomas De Schampheleire spake thusly: > >> From: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> > >> > >> check-requirements simply has to know if the results have to be sent, so > >> it can check on some extra requirements. The username and password are > >> irrelevant here. > >> This commit introduces a boolean variable do_send_results to hide these > >> details. > >> > >> Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> > >> --- > >> scripts/autobuild-run | 9 +++++---- > >> 1 file changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/scripts/autobuild-run b/scripts/autobuild-run > >> index 7497001..2ead1f2 100755 > >> --- a/scripts/autobuild-run > >> +++ b/scripts/autobuild-run > >> @@ -85,12 +85,12 @@ def check_version(): > >> print "ERROR: script version too old, please upgrade." > >> sys.exit(1) > >> > >> -def check_requirements(http_login, http_password): > >> +def check_requirements(do_send_results=False): > >> devnull = open(os.devnull, "w") > >> needed_progs = ["make", "git", "gcc", "timeout"] > >> missing_requirements = False > >> > >> - if http_login and http_password: > >> + if do_send_results: > >> needed_progs.append("curl") > >> > >> for prog in needed_progs: > >> @@ -553,8 +553,9 @@ if __name__ == '__main__': > >> check_version() > >> sysinfo = SystemInfo() > >> (ninstances, njobs, http_login, http_password, submitter) = config_get() > >> - check_requirements(http_login, http_password) > >> - if http_login is None or http_password is None: > >> + do_send_results = http_login and http_password > > > > I was told that we should no treat 'None' as 'False', or a non-empty > > string as 'True'. This should be something like:' > You can treat 'None' as 'False' and also a non empty string will value > a logical 'True'. > In a python interpreter, do bool(None) and bool('blabla'), you'll > respectively get 'False' and 'True'. > > I hope I am not the one you told you that in the first place... Not sure it was Samuel or you, but I just looked at PEP8: http://legacy.python.org/dev/peps/pep-0008/#programming-recommendations Comparisons to singletons like None should always be done with is or is not, never the equality operators. Also, beware of writing if x when you really mean if x is not None -- e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context! Regards, Yann E. MORIN. > > do_send_results = (not http_login is None) and (not http_password is None) > > > > Regards, > > Yann E. MORIN. > > > >> + check_requirements(do_send_results) > >> + if not do_send_results: > >> print "WARN: due to the lack of http login/password details, results will not be submitted" > >> print "WARN: tarballs of results will be kept locally only" > >> def sigterm_handler(signum, frame): > >> -- > >> 1.8.5.1 > >> > >> _______________________________________________ > >> buildroot mailing list > >> buildroot@busybox.net > >> http://lists.busybox.net/mailman/listinfo/buildroot > > > > -- > > .-----------------.--------------------.------------------.--------------------. > > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > > | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | > > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > > '------------------------------^-------^------------------^--------------------' > > _______________________________________________ > > buildroot mailing list > > buildroot@busybox.net > > http://lists.busybox.net/mailman/listinfo/buildroot
On Sat, Oct 18, 2014 at 12:25 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > Maxime, All, > > On 2014-10-18 12:15 +0200, Maxime Hadjinlian spake thusly: >> On Sat, Oct 18, 2014 at 12:20 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: >> > Thomas, All, >> > >> > On 2014-10-16 21:15 +0200, Thomas De Schampheleire spake thusly: >> >> From: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> >> >> >> >> check-requirements simply has to know if the results have to be sent, so >> >> it can check on some extra requirements. The username and password are >> >> irrelevant here. >> >> This commit introduces a boolean variable do_send_results to hide these >> >> details. >> >> >> >> Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> >> >> --- >> >> scripts/autobuild-run | 9 +++++---- >> >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/scripts/autobuild-run b/scripts/autobuild-run >> >> index 7497001..2ead1f2 100755 >> >> --- a/scripts/autobuild-run >> >> +++ b/scripts/autobuild-run >> >> @@ -85,12 +85,12 @@ def check_version(): >> >> print "ERROR: script version too old, please upgrade." >> >> sys.exit(1) >> >> >> >> -def check_requirements(http_login, http_password): >> >> +def check_requirements(do_send_results=False): >> >> devnull = open(os.devnull, "w") >> >> needed_progs = ["make", "git", "gcc", "timeout"] >> >> missing_requirements = False >> >> >> >> - if http_login and http_password: >> >> + if do_send_results: >> >> needed_progs.append("curl") >> >> >> >> for prog in needed_progs: >> >> @@ -553,8 +553,9 @@ if __name__ == '__main__': >> >> check_version() >> >> sysinfo = SystemInfo() >> >> (ninstances, njobs, http_login, http_password, submitter) = config_get() >> >> - check_requirements(http_login, http_password) >> >> - if http_login is None or http_password is None: >> >> + do_send_results = http_login and http_password >> > >> > I was told that we should no treat 'None' as 'False', or a non-empty >> > string as 'True'. This should be something like:' >> You can treat 'None' as 'False' and also a non empty string will value >> a logical 'True'. >> In a python interpreter, do bool(None) and bool('blabla'), you'll >> respectively get 'False' and 'True'. >> >> I hope I am not the one you told you that in the first place... > > Not sure it was Samuel or you, but I just looked at PEP8: > > http://legacy.python.org/dev/peps/pep-0008/#programming-recommendations > > Comparisons to singletons like None should always be done with is or > is not, never the equality operators. > > Also, beware of writing if x when you really mean if x is not None -- > e.g. when testing whether a variable or argument that defaults to None > was set to some other value. The other value might have a type (such as > a container) that could be false in a boolean context! > Here it's fine since we only deal with basic type. > Regards, > Yann E. MORIN. > >> > do_send_results = (not http_login is None) and (not http_password is None) >> > >> > Regards, >> > Yann E. MORIN. >> > >> >> + check_requirements(do_send_results) >> >> + if not do_send_results: >> >> print "WARN: due to the lack of http login/password details, results will not be submitted" >> >> print "WARN: tarballs of results will be kept locally only" >> >> def sigterm_handler(signum, frame): >> >> -- >> >> 1.8.5.1
On 18/10/14 00:20, Yann E. MORIN wrote: > Thomas, All, > > On 2014-10-16 21:15 +0200, Thomas De Schampheleire spake thusly: [snip] >> @@ -553,8 +553,9 @@ if __name__ == '__main__': >> check_version() >> sysinfo = SystemInfo() >> (ninstances, njobs, http_login, http_password, submitter) = config_get() >> - check_requirements(http_login, http_password) >> - if http_login is None or http_password is None: >> + do_send_results = http_login and http_password > > I was told that we should no treat 'None' as 'False', or a non-empty > string as 'True'. This should be something like: > > do_send_results = (not http_login is None) and (not http_password is None) I'm with Yann (not Maxime) on this one: if you would have an autobuild server that doesn't need a username or password, then there would be no distinction between sending or not sending results. with 'http_login is not None and http_password is not None' you can set them to the empty string to force sending. In practice, of course, it doesn't matter because we have only one autobuild server and it does require a non-empty username and password. Regards, Arnout > > Regards, > Yann E. MORIN. > >> + check_requirements(do_send_results) >> + if not do_send_results: >> print "WARN: due to the lack of http login/password details, results will not be submitted" >> print "WARN: tarballs of results will be kept locally only" >> def sigterm_handler(signum, frame): >> -- >> 1.8.5.1 >> >> _______________________________________________ >> buildroot mailing list >> buildroot@busybox.net >> http://lists.busybox.net/mailman/listinfo/buildroot >
On Sat, Oct 18, 2014 at 3:38 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 18/10/14 00:20, Yann E. MORIN wrote: >> Thomas, All, >> >> On 2014-10-16 21:15 +0200, Thomas De Schampheleire spake thusly: > [snip] >>> @@ -553,8 +553,9 @@ if __name__ == '__main__': >>> check_version() >>> sysinfo = SystemInfo() >>> (ninstances, njobs, http_login, http_password, submitter) = config_get() >>> - check_requirements(http_login, http_password) >>> - if http_login is None or http_password is None: >>> + do_send_results = http_login and http_password >> >> I was told that we should no treat 'None' as 'False', or a non-empty >> string as 'True'. This should be something like: >> >> do_send_results = (not http_login is None) and (not http_password is None) Just for the sake of it: do_send_results = (http_login is not None) and (http_password is not None) > > I'm with Yann (not Maxime) on this one: if you would have an autobuild server > that doesn't need a username or password, then there would be no distinction > between sending or not sending results. with 'http_login is not None and > http_password is not None' you can set them to the empty string to force sending. Very true, I did not think about that case. And as bonus point, like Yann said previously, it's more explicit. > > In practice, of course, it doesn't matter because we have only one autobuild > server and it does require a non-empty username and password. > > > Regards, > Arnout > >> >> Regards, >> Yann E. MORIN. >> >>> + check_requirements(do_send_results) >>> + if not do_send_results: >>> print "WARN: due to the lack of http login/password details, results will not be submitted" >>> print "WARN: tarballs of results will be kept locally only" >>> def sigterm_handler(signum, frame): >>> -- >>> 1.8.5.1 >>> >>> _______________________________________________ >>> buildroot mailing list >>> buildroot@busybox.net >>> http://lists.busybox.net/mailman/listinfo/buildroot >> > > > -- > Arnout Vandecappelle arnout at mind be > Senior Embedded Software Architect +32-16-286500 > Essensium/Mind http://www.mind.be > G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven > LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle > GPG fingerprint: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
All, On 2014-10-18 15:52 +0200, Maxime Hadjinlian spake thusly: > On Sat, Oct 18, 2014 at 3:38 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > > On 18/10/14 00:20, Yann E. MORIN wrote: > >> Thomas, All, > >> > >> On 2014-10-16 21:15 +0200, Thomas De Schampheleire spake thusly: > > [snip] > >>> @@ -553,8 +553,9 @@ if __name__ == '__main__': > >>> check_version() > >>> sysinfo = SystemInfo() > >>> (ninstances, njobs, http_login, http_password, submitter) = config_get() > >>> - check_requirements(http_login, http_password) > >>> - if http_login is None or http_password is None: > >>> + do_send_results = http_login and http_password > >> > >> I was told that we should no treat 'None' as 'False', or a non-empty > >> string as 'True'. This should be something like: > >> > >> do_send_results = (not http_login is None) and (not http_password is None) > Just for the sake of it: > do_send_results = (http_login is not None) and (http_password is not None) Yup, I forgot that '... is not ...' is preferred over 'not ... is ...' Regards, Yann E. MORIN.
[sorry, sent from non-subscribed account first, will bounce on the list] All, On Sat, Oct 18, 2014 at 7:34 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > All, > > On 2014-10-18 15:52 +0200, Maxime Hadjinlian spake thusly: >> On Sat, Oct 18, 2014 at 3:38 PM, Arnout Vandecappelle <arnout@mind.be> wrote: >> > On 18/10/14 00:20, Yann E. MORIN wrote: >> >> Thomas, All, >> >> >> >> On 2014-10-16 21:15 +0200, Thomas De Schampheleire spake thusly: >> > [snip] >> >>> @@ -553,8 +553,9 @@ if __name__ == '__main__': >> >>> check_version() >> >>> sysinfo = SystemInfo() >> >>> (ninstances, njobs, http_login, http_password, submitter) = config_get() >> >>> - check_requirements(http_login, http_password) >> >>> - if http_login is None or http_password is None: >> >>> + do_send_results = http_login and http_password >> >> >> >> I was told that we should no treat 'None' as 'False', or a non-empty >> >> string as 'True'. This should be something like: >> >> >> >> do_send_results = (not http_login is None) and (not http_password is None) >> Just for the sake of it: >> do_send_results = (http_login is not None) and (http_password is not None) > > Yup, I forgot that '... is not ...' is preferred over 'not ... is ...' > Extensive debate on just one line :) First of all, the construct 'http_login and http_password' was already in the original code, I just placed the result in a variable to allow for reuse. Secondly, regarding the quote Yann made of PEP8: Comparisons to singletons like None should always be done with is or is not, never the equality operators. --> This is not relevant in this case, according to me, since we are not using equality operators (==, !=). Also, beware of writing if x when you really mean if x is not None -- e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context! --> This part can be discussed. Arnout suggested that there may be a hypothetical server that accepts empty http logins or password. Yes, if you want to allow that, you cannot simply convert http_login in a boolean as the empty string would render False, but it's not a case I was coding for. If I see code like: do_send_results = (http_login is not None) and (http_password is not None) my first reaction would be to change it into do_send_results = http_login and http_password although indeed it's not exactly the same. So a comment should be written to make it explicit that the empty string is accepted as a valid login/password. All in all, I wonder if the empty string as login/password is a valid use case to consider, and whether the uglier code is worth that. But I'm not a difficult person, so if the consensus is to change that line, no problem! Best regards, Thomas
diff --git a/scripts/autobuild-run b/scripts/autobuild-run index 7497001..2ead1f2 100755 --- a/scripts/autobuild-run +++ b/scripts/autobuild-run @@ -85,12 +85,12 @@ def check_version(): print "ERROR: script version too old, please upgrade." sys.exit(1) -def check_requirements(http_login, http_password): +def check_requirements(do_send_results=False): devnull = open(os.devnull, "w") needed_progs = ["make", "git", "gcc", "timeout"] missing_requirements = False - if http_login and http_password: + if do_send_results: needed_progs.append("curl") for prog in needed_progs: @@ -553,8 +553,9 @@ if __name__ == '__main__': check_version() sysinfo = SystemInfo() (ninstances, njobs, http_login, http_password, submitter) = config_get() - check_requirements(http_login, http_password) - if http_login is None or http_password is None: + do_send_results = http_login and http_password + check_requirements(do_send_results) + if not do_send_results: print "WARN: due to the lack of http login/password details, results will not be submitted" print "WARN: tarballs of results will be kept locally only" def sigterm_handler(signum, frame):