nelfin / pylint-protobuf

A plugin for making Pylint aware of the fields of protobuf-generated classes
MIT License
29 stars 12 forks source link

Nested enums are not correctly handled #16

Closed mishas closed 3 years ago

mishas commented 5 years ago

Consider the following blah.proto file:

enum EnumOne {
    UNDEFINED = 0;
    ONE = 1;
    TWO = 2;
}

message Message {
    enum EnumTwo {
        UNDEFINED = 0;
        UNO = 1;
        DOS = 2;
    }
    [...]
}

And consider the following code:

import blah_pb2

blah_pb2.ONE  # This one is just fine, as expected.
blah_pb2.Message.UNO  # This one is the correct way (will run) but triggers a pylint warning.
blah_pb2.UNO  # This one is incorrect (won't run), but it doesn't trigger a pylint warning.
nelfin commented 5 years ago

Thanks for the report. I'll see what the interaction is with nested message definitions (which are handled) and nested enum definitions, which seemingly aren't.

nelfin commented 5 years ago

blah_pb2.UNO didn't warn because it turned out modules were skipped from consideration when checking attributes. I've fixed that now.

eugene-kulak commented 4 years ago

@nelfin any progress on this? this is a very common scenario for me

nelfin commented 4 years ago

@eugene-kulak: progress on pylint-protobuf? Some. Material progress on this particular issue? No, sorry. Truthfully I don't use protobufs day-to-day so I struggle to make progress on this plugin. I've just started working on it again this last week after a months-long hiatus but I can't guarantee any features or fixes by any given deadline at the moment.

nelfin commented 3 years ago

@mishas, @eugene-kulak: I believe all these cases are addressed in 0.14. See https://github.com/nelfin/pylint-protobuf/blob/6593ec1f39bd607d68e30f43b25804ca1e8d71c4/tests/test_enum.py#L119-L173

Are there any other cases you encounter around the behaviour of enum definitions that are unexpected?

nelfin commented 3 years ago

I've confirmed on my end that this is fixed, I'll close this issue in a couple of days if there's no update.

mishas commented 3 years ago

Hello @nelfin , Thanks for fixing! I didn't have the chance to test internally, but the unittests look promising. I think you can close this issue, and worst-case - I'll reopen it :)

Thanks again!

nelfin commented 3 years ago

Cheers