Inline ignoring again

[mdevaev]

Hello. I'm about #9.

I'm opening a new issue because I want to describe my use case and ask you to reconsider your decision regarding the support of inline comments like # vulture: ignore.

I am developing software that actively interacts with hardware using binary protocols. I describe individual elements of the protocol using dataclasses, for example:

@dataclasses.dataclass(frozen=True)
class Nak(Unpackable):
    reason:  int
    payload: int

    INVALID_COMMAND = 0
    BUSY            = 1
    NO_LINK         = 2
    OVERFLOW        = 3

    __struct = struct.Struct("<BB")

    @classmethod
    def unpack(cls, data: bytes, offset: int=0) -> "Nak":
        return Nak(*cls.__struct.unpack_from(data, offset=offset))

This class describes the response from the hardware and possible response options. But due to the fact that hardware often has more features than it is processed in software (for example, the payload for the error is not important to us, only the NAK itself is important), the response fields may not be used, or some constants, although defined, will not be used either.

So vulture is triggered on them. I don't want to delete the field definitions because this is actually the protocol documentation in the code, and I don't want to delete unused protocol constants for the same reason. The only way to silence vulture is to have a separate file with a list of fields, or commenting them.

It turns out that I have two places where I describe some things from the protocol: here is the class itself, and here is a separate list of unused things. However, if I start using some field or constant, I often forget to remove it from the vulture list. But the class definition file is a natural place that I often visit. And if I could describe exactly there that these fields are not used, it would be more convenient. My definition would no longer be split into two files. Like this:

@dataclasses.dataclass(frozen=True)
class Nak(Unpackable):
    reason:  int
    payload: int  # vulture: ignore

    INVALID_COMMAND = 0  # vulture: ignore
    BUSY            = 1
    NO_LINK         = 2  # vulture: ignore
    OVERFLOW        = 3  # vulture: ignore

    ...

I understand your arguments in #9, but I ask you to review this feature because it will greatly simplify my life :)

[plowman]

I agree with what @mdevaev is saying, and to add some additional context: my company is using Vulture, and in general it works really well, but the whitelist file is a continuous source of pain.

Our vulture_whitelist.py file is almost 1000 lines which change every time any whitelisted file changes, because every line number is specified in the file. So nearly every PR that's merged causes a merge conflict with any other unmerged branches because of vulture_whitelist.py. Having an inline way of ignoring these vulture warnings would be a nice way of avoiding the giant whitelist file.

The only other alternative to this, IMO, would be a whitelist file which is less specific about the line number of everything. Right now the unused code is specified as `my_variable_name # unused variable (a/b/c/my_module.py:line_number)

Even just specifying the module path like a.b.c.my_module.my_variable_name instead of including the line number would go a long way to reducing churn and merge conflicts in that file.

[jendrikseipp]

Thanks for your input! I'll think a bit more about inline comments. But regarding the problem of line numbers in the whitelist file: you can simply remove all line numbers. The comments in that file are ignored by Vulture and only meant for humans.

[mdevaev]

I'm using this file without line numbers. There is still the problem of "distributed knowledge" about unused fields. That is, I open a file with a description of structures, and from it it is not obvious to me which fields are used in the project and which are not. I will have to use a code search, or look at the list in vulture.

By the way, a similar technique with unused function arguments exists in C. When warnings are enabled, the compiler will complain about unused arguments for functions. Therefore, there are two ways to mark such variables:

int foo(int x) {
    (void)x; // This is unused
    return 0;
}
int bar (__attribute__((unused)) int y) {
    return 0;
}

While I was writing about this, I thought about the semantics of inline and that it could really be a source of knowledge about the use of fields or variables in code:

• If an unused variable is not marked as unused, then vulture will complain about it as usual.
• But if you started using a variable that was marked unused, it should complain too! Thus, the marks will not just be a list of what vulture should turn a blind eye to, they will turn into an accurate and truthful tool to help understand the semantic of variables.

If a variable has been marked and it has been started to be used, vulture forces the developer to remove the corresponding label. This is a small self-checking documentation. I find it very useful.