imporve pylint; report errors in a deterministic order

Bug #1625158 reported by Amrith Kumar
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack DBaaS (Trove)
Fix Released
Undecided
Peter Stachowski

Bug Description

Peter (peterstac) observes that the fact that the config is not in a deterministic order makes it hard to compare two config's and see what changed.

Similarly, the output from pylint (errors) are generated one file at a time and os.walk makes no guarantee of deterministic order. So we should collect all errors (across all files) and then print an ordered list for human consumption.

Revision history for this message
Amrith Kumar (amrith) wrote :

Personally, I'm not positive I understand the first use-case; i.e. you have an existing config file, you save it, and then rebuild and then diff the two files. I'd have thought you'd just run check and the output of the tool was the diff.

I however do see the value in sorting the file so that when someone submits a change that includes a change to the config, reviewers can see more easily what the change is doing.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to trove (master)

Fix proposed to branch: master
Review: https://review.openstack.org/372448

Changed in trove:
status: New → In Progress
Changed in trove:
assignee: Amrith (amrith) → Peter Stachowski (peterstac)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to trove (master)

Reviewed: https://review.openstack.org/372448
Committed: https://git.openstack.org/cgit/openstack/trove/commit/?id=a2d336de2a2275b6abd6e7933347af72363bcc03
Submitter: Jenkins
Branch: master

commit a2d336de2a2275b6abd6e7933347af72363bcc03
Author: Amrith Kumar <email address hidden>
Date: Mon Sep 19 08:26:24 2016 -0400

    improve pylint; generate errors and config in sorted order

    Since the config is not in a deterministic order makes it hard to
    compare two config's and see what changed. Personally, I'm not
    positive I understand this use-case; i.e. you have an existing config
    file, you save it, and then rebuild and then diff the two files. I'd
    have thought you'd just run check and the output of the tool was the
    diff.

    I however do see the value in sorting the file so that when someone
    submits a change that includes a change to the config, reviewers can
    see more easily what the change is doing.

    Similarly, the output from pylint (errors) are generated one file at a
    time and os.walk makes no guarantee of deterministic order. So we
    should collect all errors (across all files) and then print an ordered
    list for human consumption.

    The intent is also to make pylint voting soon (in master). the changes
    to contributing.rst and tox.ini are to make that easier. The config
    file has also been sorted in place.

    This change was motivated by an email exchange with Peter so I am
    marking him as a co-conspirator.

    The line numbers were removed from the tools/trove-pylint.config file
    as these would change whenever the line numbers in the file changed
    (since they are currently not being used in the comparison; they can
    be re-added if deemed necessary at the cost of having every 'rebuild'
    run create a different file).

    The tools/trove-pylint.config was regenerated as well, since the
    remaining two errors seem to be innocuous:

      ERROR: trove/taskmanager/manager.py 392: E1101 no-member,
      Manager.upgrade: Instance of 'BuiltInstance' has no 'upgrade' member
      (new method introduced by instance upgrade; other BuiltInstance
      member errors are already ignored.)

    and

      ERROR: trove/guestagent/datastore/experimental/postgresql/service/
      access.py 80: E1101 no-member, PgSqlAccess.list_access: Instance ofi
      'PgSqlAccess' has no '_find_user' member
      (this is due to the fact that PostgreSQL is spread over multiple
      files and pylint should cease to complain once
      https://review.openstack.org/#/c/346082/ lands.)

    Change-Id: I910c738d3845b7749e57910f76523150ec5a5bff
    Closes-Bug: #1625158
    Closes-Bug: #1625245
    Co-Authored-By: Peter Stachowski <email address hidden>

Changed in trove:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to trove (stable/newton)

Fix proposed to branch: stable/newton
Review: https://review.openstack.org/373221

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to trove (stable/newton)

Reviewed: https://review.openstack.org/373221
Committed: https://git.openstack.org/cgit/openstack/trove/commit/?id=0306df00ea845594e2c589deca68c79b399f3358
Submitter: Jenkins
Branch: stable/newton

commit 0306df00ea845594e2c589deca68c79b399f3358
Author: Amrith Kumar <email address hidden>
Date: Mon Sep 19 08:26:24 2016 -0400

    improve pylint; generate errors and config in sorted order

    Since the config is not in a deterministic order makes it hard to
    compare two config's and see what changed. Personally, I'm not
    positive I understand this use-case; i.e. you have an existing config
    file, you save it, and then rebuild and then diff the two files. I'd
    have thought you'd just run check and the output of the tool was the
    diff.

    I however do see the value in sorting the file so that when someone
    submits a change that includes a change to the config, reviewers can
    see more easily what the change is doing.

    Similarly, the output from pylint (errors) are generated one file at a
    time and os.walk makes no guarantee of deterministic order. So we
    should collect all errors (across all files) and then print an ordered
    list for human consumption.

    The intent is also to make pylint voting soon (in master). the changes
    to contributing.rst and tox.ini are to make that easier. The config
    file has also been sorted in place.

    This change was motivated by an email exchange with Peter so I am
    marking him as a co-conspirator.

    The line numbers were removed from the tools/trove-pylint.config file
    as these would change whenever the line numbers in the file changed
    (since they are currently not being used in the comparison; they can
    be re-added if deemed necessary at the cost of having every 'rebuild'
    run create a different file).

    The tools/trove-pylint.config was regenerated as well, since the
    remaining two errors seem to be innocuous:

      ERROR: trove/taskmanager/manager.py 392: E1101 no-member,
      Manager.upgrade: Instance of 'BuiltInstance' has no 'upgrade' member
      (new method introduced by instance upgrade; other BuiltInstance
      member errors are already ignored.)

    and

      ERROR: trove/guestagent/datastore/experimental/postgresql/service/
      access.py 80: E1101 no-member, PgSqlAccess.list_access: Instance ofi
      'PgSqlAccess' has no '_find_user' member
      (this is due to the fact that PostgreSQL is spread over multiple
      files and pylint should cease to complain once
      https://review.openstack.org/#/c/346082/ lands.)

    Change-Id: I910c738d3845b7749e57910f76523150ec5a5bff
    Closes-Bug: #1625158
    Closes-Bug: #1625245
    Co-Authored-By: Peter Stachowski <email address hidden>
    (cherry picked from commit a2d336de2a2275b6abd6e7933347af72363bcc03)

tags: added: in-stable-newton
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/trove 6.0.0

This issue was fixed in the openstack/trove 6.0.0 release.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/trove 7.0.0.0b1

This issue was fixed in the openstack/trove 7.0.0.0b1 development milestone.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/trove 6.0.0

This issue was fixed in the openstack/trove 6.0.0 release.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.