Breaking down C0111(Missing Docstring) into multiple "Convention" messages

[Pierre-Sassoulas]

This is an incomplete pull request in order to see more easily what needs to be changed in order to implement #1164 after a long period of inactivity.

[AWhetter]

This is a great change. I love the idea!
It's not something to address in this pull request, but perhaps in the future we can split out an extra message for magic methods and another for nested classes or functions. I think there will be times that people will want to turn those messages off especially, but still get messages for everything else.

I've made a few small comments but there's nothing major. It would be great to get them fixed before merging but it's not a problem if they don't get fixed.
Also the tests should now be fixed so hopefully they'll pass if you make a change to this pull request.

[Pierre-Sassoulas]

@AWhetter thank you for reviewing and approving the changes. I fixed the spelling in the github interface. I coded that a long time ago and had the same reflexion than you when seeing the "ime" naming, it definitely needs a refactor. So I'll make the other change too, but only when I have an IDE and git available.

Regarding the tests, they're not passing because the refactor required in order to have multiple old messages with the same name is not done yet :)

[PCManticore]

Thanks @Pierre-Sassoulas This looks great! I left a couple of small comments, but nothing that's related to the logic, which looks straightforward. Also if you can, please also add a description in the What's New document for this release, 2.0 rst, which can go a bit more in depth on what we did here (split the missing-docstring, mention the fact that the old disable/enable can still work with missing-docstring and mention the new added messages`

[Pierre-Sassoulas]

There is a function, called "MessageStore.check_message_id(msgid)" (~= line 850 in utils.py). It is used outside of MessageStore a dozen of times. It return a MessageDefinition instance when you give a msgid and raise an error if there is no corresponding message for the msgid. It does not check anything. So I think it should actually be called "get_message_from_msgid(msgid)". Can I refactore this or did I miss something?

[PCManticore]

From the sound of it, you can refactor it, but let's do it in a separate PR

…

On Wed, May 9, 2018, 09:54 Pierre Sassoulas ***@***.***> wrote: There is a function, called "MessageStore.check_message_id(msgid)" (~= line 850). It is used outside of MessageStore a dozen of times. It return a MessageDefinition instance when you give a msgid and raise an error if there is no corresponding message for the msgid. It does not check anything. So I think it should actually be called "get_message_from_msgid(msgid)". Can I refactore this or did I miss something? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2036 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbWpiv3xkbCwVrmBga8sG-6lxWa5ZXHks5twvUBgaJpZM4Tha9_> .

[Pierre-Sassoulas]

Regarding the refactor in MessageStore. Let's take an example :

   'W1235': (
        'Child 1', 'child-one', 'Child one description.',
        {'old_names': [('W1234', 'mother')]}
    ),
    'W1236': (
        'Child 2', 'child-two', 'Child two description',
        {'old_names': [('W1234', 'mother')]}
    )

Right now this raises an error and what I think is stored in MessagesStore is this:

message = {
W1235 : # message_definition_for W1235,
W1236: # message_definition_for W1236,
}

alternate_name = {
W1234: W1235,
# W1234: W1236, Impossible
}

I have a feeling that we could construct a message for the old name and link it for every child symbol/msgid :

message = {
W1235 : # message_definition_for W1235,
W1236: # message_definition_for W1236,
}

alternate_name = {
W1234 : # message_definition_for W1234
}

But I don't know how to construct the mother message for W1234 in this case. Do we take W1235 message definition ? W1236 ? Something new ?

So the other way would be to make a (very big?) refactor to permit MessagesStore to return a list of message definitions :

message = {
W1235 : # [message_definition_for W1235],
W1236: # [message_definition_for W1236],
W1234 : # [message_definition_for W1235, message_definition_for W1236],
}

Or maybe a list only if its an alternate name :

message = {
W1235 : # message_definition_for W1235,
W1236: # message_definition_for W1236,
}

alternate_name = {
W1234 : # [message_definition_for W1235, message_definition_for W1236],
}

Any thoughts @PCManticore ?

[Pierre-Sassoulas]

@PCManticore This is ready for review but everything will be easier if we first resolve the smaller chunks. This MR is based upon #3021, #3041, #2992, and #3013, let's merge them first (in this order).

[coveralls]

Coverage increased (+0.05%) to 90.033% when pulling f1bbf64 on Pierre-Sassoulas:master into 41c9522 on PyCQA:master.

[coveralls]

Coverage increased (+0.05%) to 90.033% when pulling f1bbf64 on Pierre-Sassoulas:master into 41c9522 on PyCQA:master.

[coveralls]

Coverage increased (+0.05%) to 90.033% when pulling f1bbf64 on Pierre-Sassoulas:master into 41c9522 on PyCQA:master.

[coveralls]

Coverage decreased (-0.02%) to 90.034% when pulling 44fbb52 on Pierre-Sassoulas:master into ae81e3b on PyCQA:master.

[Pierre-Sassoulas]

@PCManticore this is the final merge request for #1164. In hindsight it looks like I could have chosen an easier first issue :)

[PCManticore]

@Pierre-Sassoulas Alright, this is ready! Thank you so much for the effort you put into getting this change in. 🙇 It took a tremendous amount of effort and refactoring to get to this point, and I appreciate your work and enthusiasm in making these three separate checks a reality.