search

pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.33k stars 1.14k forks source link

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

Closed Pierre-Sassoulas closed 5 years ago

Pierre-Sassoulas commented 8 years ago

As of now C0111 can be multiple thing :

I wanted to disable "Missing module docstring" specifically, because I use one class by module and documenting the module when the class is already documented look like duplication of information. So in order to be able to do that, I think it would be nice if there was a distinction between those various C0111.

Apparently I'm not the only one who want to do this : see this question on stackOverflow.

Seeing the message it gives, it seems that Pylint can already do the distinction and that the biggest problem would be retro compatibility and preventing users that want to get rid of all C0111 in one disable from being forced to disable 4 of them.

Maybe add C0111-A to C0111-D so you can use C0111 as a generic disabler ? Any idea ?

Regards,

PCManticore commented 8 years ago

Sounds fair to me.

The backwards compatibility issue can be taken care of by using old_names option for each of the new messages. For instance, let's say we are adding missing-class-docstring, missing-function-docstring and missing-module-docstring messages. Each one of it would get a old_names parameter in the message definition, pointing to missing-docstring, which means that disabling missing-docstring will disable all of these.

Pierre-Sassoulas commented 8 years ago

If I understand you correctly, what we should do is :

I have two questions :

Also could you assign this features to me ? I'm new to the pylint codebase, but it sounds like it could be my first contribution. It would still be nice if you have tips for this :).

PCManticore commented 8 years ago

Yes, I meant exactly that. This way we ensure backwards compatibility.

Regarding the first question, I thought about the same, it might not make sense to have them separated, so I am fine with a missing-function-docstring that applies to both functions and methods. Regarding the code name, you can check in the checker's file what is the next unassigned number and take one from there. It does not matter which one is, as long as it respects the internal consistency of the checker (a checker with C11... should have all its messages starting with C11). The ids are less important than symbol names, since the latter are more intuitive.

Sure thing, the issue is assigned to you. You can definitely start seeing where the message lies and how easy is to split it. The next thing to do is to have some tests, with having finally a changelog entry, a what's new entry, as well as adding yourself to contributors. But let's do the first step first and we will see how it progresses. Let me know if you have any question, you can just write them here and I'll try to respond in a timely manner.

Pierre-Sassoulas commented 7 years ago

Hi again,

I tried a few things and I now have some questions.

Making tox (kinda) work took me some time. I used https://pylint.readthedocs.io/en/latest/development_guide/contribute.html and http://tox.readthedocs.io/en/latest/ as documentation. Is there something else ? I still have some difficulties making tox run correctly with all the python versions because my distribution does not provide python 3.3, 3.4, and 3.6 by default. Is there some kind of procedure to recover all the python versions to run tests or should I just install manually each of them ?

Also, it seem that I can't set multiple message inside "DocStringChecker", I have an error message saying "Message alternate name 'missing-docstring' is already defined". Can I define multiple "old_name" with the same code name within the same Checker ? (I tried to set a different name for each but it does not seem to work : https://github.com/Pierre-Sassoulas/pylint/commit/03c2806bfc5189b90de5c3ad9bc23d9a4cd85ac8). Or should I inherit from DocstringChecker and create a child DocstringCheckers for each new message so I only have one ?

Thanks in advance,

PCManticore commented 7 years ago

Hi Pierre,

What problems did you stumble upon with tox? We have it configured to skip missing interpreters automatically, so this should not be a problem: https://github.com/PyCQA/pylint/blob/master/tox.ini#L3 You can definitely run tests justs for one or another interpreter, as in tox -e py35.

Regarding your second problem, indeed, it seems that it does not work as is, we will probably need some modifications around this function: https://github.com/PyCQA/pylint/blob/2beb5e650d689d4903676b5ca6d82a1806bbbe79/pylint/utils.py#L713, but in particular, around here: https://github.com/PyCQA/pylint/blob/2beb5e650d689d4903676b5ca6d82a1806bbbe79/pylint/utils.py#L707. We need to make sure that we can register multiple new messages for an old message, as it is the case here. Do you want to tackle this refactoring?

Pierre-Sassoulas commented 7 years ago

Hello,

For tox, first pip gave me a lot of "Could not find a version that satisfies the requirement", but I got it to run somehow. Second, I thougth I had to run tests for all the interpreters which could take some time to download, compile and install. But reading your answer, it seems acceptable to just run tests for 2.7 and 3.5 before doing a pull request ?

Thank you for the tips on where it need tweaking, I will try to tackle it (although it could take some time, with end of the year planning) even if it's getting a little harder than anticipated :) May I ask questions here again if i'm stuck ?

Regards,

PCManticore commented 7 years ago

Aaa, I see now what you meant. We don't actually install those interpreters, but we rely on them existing already. If they don't exist, they are skipped due to skip_missing_interpreter option. You can definitely run tests for what interpreters you have and when you send a pull request, our CI is going to test it with all the interpreters.

Sure thing, don't worry, we are not under a time constraint. You can definitely ask questions here if you have. I usually hang out on IRC from time to time, on #pylint under #freenode. Or you can just shoot me an email.

Fity commented 7 years ago

Excuse me, how is this issue going now?

Pierre-Sassoulas commented 7 years ago

Hello,

I made the change to break down the missing-docstring given by Pylint into multiple messages : https://github.com/Pierre-Sassoulas/pylint/commit/50f3ad4380e641e32e86ba7dfd0b79bb08bb38d9. At least I think the change should be made this way. As discussed above, there was multiple checks to prevent duplicate msgid/symbolic name. So there is a tad of refactoring to do. @PCManticore can you confirm this should not raise an error and that I should refactor MessageStore in order for this to work ?

What I'm trying to do is:

I started to refactor and tests are passing for this commit : https://github.com/Pierre-Sassoulas/pylint/commit/8132e9498f6908d6654a7ab94b35d54c2209504d.

But I have a problem with the way alternate name is constructed right now 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 is done in alternate name is this:

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

I have a feeling that I could :

Any thoughts on the best way to refactor MessageStore ?

Also, updating tests takes time, because I have an SSL error using tox that is pretty much preventing me from launching tests (except at work, where I'm just launching tests, because pylint is pretty far from my core mission). Any hint, about how to disable ssl inside tox would be appreciated.

Pierre-Sassoulas commented 7 years ago

@PCManticore any thoughts on the two ideas for redesigning MessageStore I gave in the message above ? I have a feeling that this will be a big refactor and i don't know how much it will impact pylint, so I'd appreciate your input before going for it.

Thanks in advance,

pawamoy commented 7 years ago

Just a note: if you want to run tox with all Python versions locally, you can install these versions with pyenv, it's really easy to install and use:

# make sure to have these libs to compile Python with
sudo apt install -y libssl-dev openssl zlib1g-dev sqlite3 libsqlite3-dev libbz2-dev bzip2

# download pyenv
git clone https://github.com/pyenv/pyenv.git ~/.pyenv

# put it in your path and initialize it, in .bashrc for example
export PATH="${HOME}/.pyenv/bin:${PATH}"
eval "$(pyenv init -)"

# now install all the versions you need (it can indeed take some time)
pyenv install 3.5.3
pyenv install 3.6.1
pyenv install 3.7-dev

# and last but very important step, run this once:
pyenv global system 3.5.3 3.6.1 3.7-dev
# the order is important, list all the installed versions you want to use
# thanks to that, tox will find all the executables
# "system" should always be first so we keep normal behavior when not in a virtualenv
henryJack commented 7 years ago

Any updates on this?

Pierre-Sassoulas commented 7 years ago

Hello @henryJack,

In my branch there is the change in "missing docstring" and the change in the pylint test for the necessary MessageStore refactor. This refactor would have potentially huge ramifications, so I was waiting for the input of @PCManticore or someone experimented before doing something too drastic. It probably got lost in the notifications during the 1.8.0 release.

If someone want to try to refactor MessageStore without senior supervision, the tests in my branch should permit to do that, then we could see if the change made do "missing docstring" are working as intended.

onwsk8r commented 7 years ago

Note that in the interim, there is a quick fix for this problem that I've been using as I watch this thread (coding: ... left in for clarity)-

# -*- coding: utf-8 -*-
# pylint: disable=missing-docstring

# pylint: enable=missing-docstring
import some_stuff

class FriendOfPylint(object):
    ...
henryJack commented 7 years ago

@Pierre-Sassoulas and @onwsk8r - I have moved over to using flake8 and flake8_docstrings - I have personally found it much more smooth sailing than pylint

unittolabs commented 6 years ago

I'm sorry for question but has any movement around this issue?

Pierre-Sassoulas commented 6 years ago

Sorry, nothing changed since the 30 Sep 2017 answer. I realize this has been a long time and a lot of person are interested in the feature. @PCManticore could you assign someone to review my previous questions if you don't have the time yourself ?

PCManticore commented 6 years ago

Hey @Pierre-Sassoulas and the rest of the folks. I'm sorry that this flew totally under my radar. Regarding assigning someone to review, it all depends on the interests of the contributors, we're not an agile team to assign an issue or PR to some contribuor, unless it's already their field of expertise in the project.

Now for your questions, Pierre, I would say the second one would the approach we should take, as this seems the most convenient way for handling the already existing codes, e.g if folks still use # pylint: missing-docstring, we'd need to get the corresponding newer symbolsmissing-class-docstring and friends. I also believe that this change could be encapsulated just to utils.py, MessagesStore and MessageHandlingMixin, why would we need to use a list of message for every alternate name and for messages too? I'm probably misunderstanding what you meant, but I imagine that's the case, that we'll have to use a mapping from old ids to newer symbols. There seems to be some places in utils.py where the expectation of one alternative name per msg id needs to change.

PCManticore commented 6 years ago

@Pierre-Sassoulas if you have the branch, feel free to send a PR, it will be lot easier to see what needs to be changed.

Pierre-Sassoulas commented 6 years ago

@PCManticore OK, I don't remember the detail very well. The branch is far from passing tests and I'll need to rebase first but let's start somewhere. :)

Pierre-Sassoulas commented 6 years ago

@PCManticore, I've done the rebase and created PR #2036. Re-reading the PR it seem that I added and refactored some tests, so if the tests pass the change to C0111 should work. Do not hesitate to comment and ask question on the PR if something is'nt clear.

PCManticore commented 6 years ago

Hey @Pierre-Sassoulas Thanks for sending that PR! I'll take a look! I hope that we'll get this in 2.0 after such a long time.

tillahoffmann commented 6 years ago

Interestingly, the workaround proposed by @onwsk8r has stopped working for me when upgrading pylint from 1.9.3 to 2.0.0. But only for some files and not others even though they have the same header. Full log of the test run is here. The relevant PR is https://github.com/spotify/pythonflow/pull/36.

feluelle commented 6 years ago

Hey, is there any update on this feature? When it will be coming?

PCManticore commented 6 years ago

Hey @feluelle The honest answer is that I don't know. There are currently two PRs in flight: https://github.com/PyCQA/pylint/pull/2262 and https://github.com/PyCQA/pylint/pull/2036. The first one is ready to merge and I'll do that this week. The second one might depend on @Pierre-Sassoulas and his availability.

Pierre-Sassoulas commented 6 years ago

Hi @feluelle.

In https://github.com/PyCQA/pylint/pull/2262, we made the whole code able to handle getting multiple messages instead of one. Let's see how the code review goes, but once this is validated we'll just have to refactor a single class (MessageStore). We now have the situation under control with a clear limited scope, very far from the beginning.

Also, their was a long interruption because I switched job and the new place use flake8, so I had less time and incentive to implement this. Now codacy is using pylint, I got pestered about module docstrings again, and worst yet : my pet project was ranked B instead of the A+ it deserve. So I'm super motivated.

It should be done before 2019 :)

feluelle commented 6 years ago

Thank you @PCManticore and @Pierre-Sassoulas

In #2262, we made the whole code able to handle getting multiple messages instead of one. Let's see how the code review goes, but once this is validated we'll just have to refactor a single class (MessageStore). We now have the situation under control with a clear limited scope, very far from the beginning.

I like that 👍

It should be done before 2019 :)

That would be awesome 👍

michaelreneer commented 5 years ago

@Pierre-Sassoulas How is this coming along? Are you just waiting on #2036?

Pierre-Sassoulas commented 5 years ago

Hi @michaelreneer, since december we merged multiple big pull requests related to this change, the ongoing one being #2809. Code and code reviews takes a long time because there is a lot of changes as we're modifying the internal message storing classe and its affecting every other checkers (legacy checkers or otherwise).

I'm still on it :)

michaelreneer commented 5 years ago

@Pierre-Sassoulas Sounds great! It sounds like the fastest way to me to be notified when this is fixed is to watch this issue. Thanks BTW.

dreamalligator commented 5 years ago

Love seeing that this is progressing, thank you!

One wish list item after this first step is implemented is that directory-level rules can be implemented as a follow up. I write this here instead of a separate subject on that, is that such will be much more useful after this is done.

Pytest users typically arent going to want to add module docstrings, and it is just my opinion, but I think the majority also will want to disable function docstrings as a requirement, yet still have these in place for their whole project. I only write them in my unit tests when I actually need to explain something. In the meantime though I'll be disabling for whole project unfortunately.

One hacky workaround is enabling or disabling this rule via commandline argument. So, maybe your text editor will have differnt rules, but if you run pylint against your tests dir, your CI tests (such as in Travis-CI) can at least catch this.

PCManticore commented 5 years ago

Alright thanks, @Pierre-Sassoulas put a lot of work and effort and it finally paid off, we now have three separate checks for missing-*-docstring. This is going to be part of pylint 2.4 when it launches later this month.