xZise / flake8-string-format

Check that indexed parameters are used in strings
MIT License
20 stars 5 forks source link

P103 in regex #4

Open jayvdb opened 9 years ago

jayvdb commented 9 years ago

link_regex = re.compile(r'\[\[(?P<title>[^\]|[<>{}]*)(\|.*?)?\]\]')

produces

./pywikibot/__init__.py:643:25: P103 other string does contain unindexed parameters

xZise commented 9 years ago

This is intentional. As I'm using ast to detect strings it's not possible to me to detect raw-strings and it's basically impossible to know that re.compile won't do any funny str.format usage. For example:

class C(object):
    def compile(self, s):
        return s.format(42)
re = C()
link_regex = re.compile(r'\[\[(?P<title>[^\]|[<>{}]*)(\|.*?)?\]\]')

And while you may argue that re.compile is in most cases not using str.format there could be other cases where it's forwarding the string to re.compile and then it's not a re.compile() call directly.

So while I'd prefer to not detect these strings, I don't think there is any sensible solution without implementing #noqa.

jayvdb commented 9 years ago

A whitelist including re.compile would be useful. Anyone replacing re.compile would need to handle {} properly before using str.format.

If for example it was:

__all__ = ('foo', )
link_regex = r'\[\[(?P<title>[^\]|[<>{}]*)(\|.*?)?\]\]'
foo = re.compile(link_regex)

... the plugin could check all uses of link_regex within the module to ensure it is only used in whitelisted function calls.

This would provide users with a nicer workaround than noqa: wrap strange uses of {} with a function which can be added to the whitelist.

xZise commented 9 years ago

Well some wonky whitelist which would ignore the code from the opening post might be reasonably easy. But tracking variables could be quite a bit more complicated. As I said I'm using ast so it won't actually interpret the code… at least not to the point that I know that link_regex in line 3 was defined via line 2. And anyway… that code is still legitimate to P103 as someone might import that module and use link_regex directly.

dvorapa commented 6 years ago

Well, in the case of re: r'{}', these can be escaped (or not?): r'\{\}'. But there is more major issue when unittest wants to test, whether my_func will change the string containing these brackets:

# my_func can for example add closing bracket if it is missing
s='some {} string'
assertEqual(my_func(s), 'some {} string')

This will complain on two lines. How to solve the issue like this?

xZise commented 6 years ago

Personally I think in a case like yours @dvorapa, the P103 should be ignored. It causes many false flags because it is applied to literally every string which isn't used immediately (aka in the format of "...".format(...)). And in cases like yours I'd say that (if it's to annoying) ignoring P103 is a vailable strategy.

But maybe you got an idea to improve that error code so that you can ignore it more constrained. For example spliting it up into different types, that there are different error codes when you assign a string and when you use it as a parameter. But, as evident by your case, they would produce the same false flags.