jonathanrdelgado / SublimeTodoReview

A SublimeText plugin for reviewing todo (and other) comments within your code.
MIT License
338 stars 46 forks source link

prevent plugin crash if `forms` is NoneType #133

Closed nsfmc closed 9 years ago

nsfmc commented 9 years ago

i'm not totally clear on why, but it's possible that forms may be NoneType and todoreview will error out while indexing files. this allows drawheader to bail correctly.

feel free to disregard if you have some other fix for this or if it's part of some other issue

jonathanrdelgado commented 9 years ago

I'm all for merging, but I'm not sure how you are getting forms to be none in this context.

Can you give me a little bit more information on how that occurs? I've never heard of this being an issue. Did you set it to false in your config or something?

nsfmc commented 9 years ago

good question. first things first: i'm running the dev channel build 3095.

i did not set my config to false, here's what it is:

{
  "patterns": {
    "TODO": "TODO[\\s]*?:[\\s]*(?P<todo>.*)$",
    "Mine": "(TODO|todo)\\s*\\(marcos\\)\\s*:\\s*(?P<mytodos>.*)$",
    "FIXME": "(FIXME|fixme)\\s*\\(marcos\\)\\s*:\\s*(?P<myfixmes>.*)$"
  },
  "exclude_folders": [
    "*.git*",
    "*node_modules*",
    "*build*"
  ],
  "exclude_files": [
    "*.sublime-workspace",
    "*.sublime-project",
    "*.log"
  ],
}

but what i am noticing is that if i change draw_header to look like

    def draw_header(self):
        forms = settings.get('render_header_format', '%d - %c files in %t secs')
        datestr = settings.get('render_header_date', '%A %m/%d/%y at %I:%M%p')

        print(settings)
        print(forms)
        print(datestr)
        print('%s\n' %type(forms))
        if not forms:
            return

then every once in a while, i'll run it a few times in a row and see the following:

<sublime.Settings object at 0x104714ed0>
%d - %c files in %t secs
%A %m/%d/%y at %I:%M%p
<class 'str'>

<sublime.Settings object at 0x104803a50>
None
None
<class 'NoneType'>

which makes me think it's likely that the settings object isn't stable in the dev channel. i'm also able to occasionally crash sublime by running TodoReview: Project Files on the todoreview repo with the above config. i doubt it's anything to do with the config and more likely than not some combination of dev channel instability pushing up against TodoReview's pretty reasonable (imo) expectation that things like settings.get not return null values. ¯(ツ)

jonathanrdelgado commented 9 years ago

Yeah, sublimetext has some really odd stuff. That blows my mind that it actively ignores the set default within the function call.

Thanks for the PR, will push an update this week, have some other patches as well. Please remind me if I don't get to it.

jonathanrdelgado commented 9 years ago

Note to self on this: How does this affect setting this to false in config or having an empty string? Do we need two sets of logic to handle both?