Pull requests

#67 Merged
Repository
carandraug carandraug
Branch
default
Repository
scons scons
Branch
default

Allow NoneType for CheckContext.Result

Author
  1. carandraug avatarcarandraug
Reviewers
Description

Hi

this patch also checks for NoneType as valid result (it will print no). I stumbled upon this because GetOption will return nothing if the option was not specified. I was doing something like

def CheckEmail (context, email):
    context.Message("Checking e-mail address...")
    is_ok = GetOption ("email")
    context.Result (is_ok)
    return is_ok

Even if the patch is not accepted, someone should fix the documentation which does not mention boolean as valid.

Carnë

Comments (6)

  1. Gary Oberbrunner

    Thanks, Carandraug. Could you add a test for this? Probably test/Configure/custom-tests.py would be a good place to add it. And of course a doc patch would also be welcome! :-)

  2. carandraug author

    Hi Gary

    a doc patch will not be necessary if this is accepted. What I meant was that this commit also adds to the documentation that a boolean is a valid class (previously undocumented) and if the fix got rejected for some reason, a separate would have to be done for it.

    Anyway, I have been thinking about this since I made the change and I wish to further change this. I may be seeing Result in the wrong light, but in my mind it's not unlike an if block and we shouldn't restrict it to certain classes. Leave it to the user to pick anything that can be evaluated as true or false and only act special in the case of a string. If you agree, I can submit another patch for something like:

    if res:
      if isistance(res, str):
        text = res
      else:
        text = "yes"
    else:
      text = "no"
    

    What do you think?

  3. Gary Oberbrunner

    That is better, I agree. It will also treat empty lists and hashes as "no" which I think is fine. (Alternatively, list types could throw an exception, but I think that's not necessary.) Feel free to update your pull request.

  4. carandraug author

    I have updated the pull request. What I implemented is a bit different that what I mentioned on the previous comment, it won't check for true/false if it's a string. This is compatible with the previous behaviour for strings (empty strings, display empty string, not the word "no").

  5. carandraug author

    Not really. I tried to but I can't my head around the file you mentioned. Here's an attempt at it which I though should work but does not.

    Note that removal of int() from the test that was already there should be done anyway. The patch on the pull request makes it no longer necessary.

Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.