79

I have just tried to lint some code with Pylint, and the last remaining error is

R0902: too-many-instance-attributes (8/7)

I understand the rationale behind limiting the number of instance attributes, but seven seems a bit low. I also realise that the linter should not have the last word. However, I would like to know what I should be doing instead of:

def __init__(self, output_file=None, output_dir=None):
    """
    Set the frobnicator up, along with default geometries
    """

    self.margin = 30

    self.pos = [0, 0]
    self.sep = [5, 5]

    self.cell = [20, 20]

    self.frobbr = library.Frobbr()

    page = self.frobbr.get_settings('page')

    self.lim = [page.get_width() - self.margin,
                page.get_height() - self.margin]

    self.filename = output_file
    self.moddir = output_dir

Should I package the geometries up into a dict, do something else to stop Pylint complaining, or just ignore it (which I don't really want to do)?

Zero Piraeus
  • 56,143
  • 27
  • 150
  • 160
Inductiveload
  • 6,094
  • 4
  • 29
  • 55
  • 8
    This might be a better fit on CodeReview. Also, consider using tuples for fixed-sized values like the position. – Colonel Thirty Two Jun 26 '14 at 15:29
  • 3
    You could always combine `self.moddir` and `self.filename` into an attribute named `self.output_path`. It could either be a string such as `os.path.join(self.moddir, self.filename)` or a tuple of `(self.moddir, self.filename)`. –  Jun 26 '14 at 15:37
  • 1
    Is there anything I could add to my answer to this question to get it accepted, @Inductiveload? I wouldn't normally ask, but this Q/A pair seems to have been helpful to a decent number of people, and the checkmark might help to reassure other visitors that it's a reasonable approach. – Zero Piraeus Apr 03 '18 at 12:52
  • No, I don't know why that was never accepted way back when, it was a good answer. Sloppy maintenance by me, I suppose! – Inductiveload Apr 03 '18 at 15:19

4 Answers4

114

A linter's job is to make you aware of potential issues with your code, and as you say in your question, it should not have the last word.

If you've considered what pylint has to say and decided that for this class, the attributes you have are appropriate (which seems reasonable to me), you can both suppress the error and indicate that you've considered the issue by adding a disabling comment to your class:

class Frobnicator:

    """All frobnication, all the time."""

    # pylint: disable=too-many-instance-attributes
    # Eight is reasonable in this case.

    def __init__(self):
        self.one = 1
        self.two = 2
        self.three = 3
        self.four = 4
        self.five = 5
        self.six = 6
        self.seven = 7
        self.eight = 8

That way, you're neither ignoring Pylint nor a slave to it; you're using it as the helpful but fallible tool it is.

By default, Pylint will produce an informational message when you locally disable a check:

 Locally disabling too-many-instance-attributes (R0902) (locally-disabled)

You can prevent that message from appearing in one of two ways:

  1. Add a disable= flag when running pylint:

    $ pylint --disable=locally-disabled frob.py

  2. Add a directive to a pylintrc config file:

    [MESSAGES CONTROL]
disable = locally-disabled
Zero Piraeus
  • 56,143
  • 27
  • 150
  • 160
  • Other than the typeo, this is what I was looking for. Note that pylint will report that you are ignoring this setting, so :\ it takes the same amount of effort as a developer to ignore the warning or the notice. – ThorSummoner Feb 01 '15 at 03:35
  • 12
    @ThorSummoner - Sure, but if you live for the 10.00/10 score, it makes all the difference! :-) – MrWonderful Jun 12 '15 at 21:45
  • 3
    It makes a difference on CI as well, since it affects pylint's exit code – Spain Train Jul 06 '17 at 17:04
  • 3
    What may be the potential issue for having too many local variables or instances? @Zero Piraeus – alper May 01 '20 at 21:47
  • 2
    @alper This is a more generic "code smell"; it's not wrong in itself but it's a simple (inaccurate) way to measure complexity. If a class is too complex then *perhaps* you are doing too much in one class. So when the linter complains about this, it's a good reminder to check if the class is just responsible for one thing, or if it can be split. – Philip Couling Sep 18 '20 at 12:45
35

This is an ideological objection, but personally I tend to try to make these kind of changes as universal as possible. If 7 is not enough instances in one file, and I choose to allow it here, why not everywhere? I don't always make changes to the lint rules across the board, but I at least consider it. To that end, if you want to make an across the board change, in your .pylintrc file change max-attributes=7 in the DESIGN section.

Since I think 7 is a little low across the board, I changed:

[DESIGN]
max-attributes=7

to

max-attributes=12
Nick T
  • 25,754
  • 12
  • 83
  • 121
Bob
  • 1,779
  • 3
  • 22
  • 35
  • 2
    I can follow your argumentation, but I have a slightly different approach. why not everywhere? because I want to keep exceptional classes to exceptional, and not allow more than necessary in general. However, the question stays valid, so my disable pylint comments are always followed by a commment explaining why it was necessary for the class to have that many attributes. – Ali Rasim Kocal Dec 13 '17 at 10:46
  • Yeah I agree, I use a mix of both approaches. But when I do have to make an individual exception, I always evaluate whether I should make it everywhere too (and usually favor universal exceptions over oneoffs unless there is a super compelling reason not too). – Bob Feb 21 '18 at 14:15
8

The answer from Zero Piraeus is a good one. That said, since you provide little context to your init method, not even a real class name, it is difficult to be affirmative, but I would say filename and moddir have nothing to do aside margin, position, etc.

IO operations are often best isolated into functions rather than put into methods. Their are often many different formats and serialization options, and most of the time you do not want to mix them with your object logic (methods). It is easier to add a new IO function that takes some file, string, blob or whatever and returns the object encoded into it than it is to maintain an object that has many methods that handle many different IO operations.

NChauvat
  • 89
  • 4
2

I would disable this message altogether by adding too-many-instance-attributes to the project's pylintrc or .pylintrc file as in the example below:

[MESSAGES CONTROL]
disable=
    locally-disabled,
    locally-enabled,
    logging-format-interpolation,
    no-init,
    too-few-public-methods,
    too-many-instance-attributes,  # <-- Ensure at least this entry.
    fixme
Asclepius
  • 57,944
  • 17
  • 167
  • 143