[Twisted-Python] twistedchecker - Names of constant which are used as configuration options
Hi, Why reviewing a patch I got into this error from twistedchecker. This is old code, just that someone it was touched by recent changes, C0103:574,12:NetstringReceiverTests.test_buffer: Invalid name "MAX_LENGTH" for type attribute (should match ((([a-z_])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$) The code is like this, where TestNetstring is a line receiver protocol... so MAX_LENGTH act as a configuration option. a = TestNetstring() a.MAX_LENGTH = 699 What to do in this case and how I can have this code accepted by twistedchecker? Thanks! -- Adi Roiban
On 06:45 pm, adi@roiban.ro wrote:
Hi,
Why reviewing a patch I got into this error from twistedchecker. This is old code, just that someone it was touched by recent changes,
C0103:574,12:NetstringReceiverTests.test_buffer: Invalid name "MAX_LENGTH" for type attribute (should match ((([a-z_])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$)
The code is like this, where TestNetstring is a line receiver protocol... so MAX_LENGTH act as a configuration option.
a = TestNetstring() a.MAX_LENGTH = 699
What to do in this case and how I can have this code accepted by twistedchecker?
Fix twistedchecker. MAX_LENGTH doesn't violate the Twisted naming convention. Jean-Paul
On 24 January 2015 at 19:23, <exarkun@twistedmatrix.com> wrote:
On 06:45 pm, adi@roiban.ro wrote:
Hi,
Why reviewing a patch I got into this error from twistedchecker. This is old code, just that someone it was touched by recent changes,
C0103:574,12:NetstringReceiverTests.test_buffer: Invalid name "MAX_LENGTH" for type attribute (should match ((([a-z_])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$)
The code is like this, where TestNetstring is a line receiver protocol... so MAX_LENGTH act as a configuration option.
a = TestNetstring() a.MAX_LENGTH = 699
What to do in this case and how I can have this code accepted by twistedchecker?
Fix twistedchecker. MAX_LENGTH doesn't violate the Twisted naming convention.
Thanks. Found the report: https://github.com/twisted/twistedchecker/issues/52 Hope it will be easy to fix, -- Adi Roiban
On 24 Jan, 07:51 pm, adi@roiban.ro wrote:
On 24 January 2015 at 19:23, <exarkun@twistedmatrix.com> wrote:
On 06:45 pm, adi@roiban.ro wrote:
Hi,
Why reviewing a patch I got into this error from twistedchecker. This is old code, just that someone it was touched by recent changes,
Also: pre-existing errors like this one should not block new development work being done. It is *not* a requirement that a patch fix all nearby but unrelated problems like this one. New and changed code should conform to the standard and try not to introduce new errors. If the "errors" being introduced are due to bugs in a tool that reports errors, the bugs should be fixed. Existing code *near* new or changed code can and should be left alone. Jean-Paul
On 25 January 2015 at 01:01, <exarkun@twistedmatrix.com> wrote:
On 24 Jan, 07:51 pm, adi@roiban.ro wrote:
On 24 January 2015 at 19:23, <exarkun@twistedmatrix.com> wrote:
On 06:45 pm, adi@roiban.ro wrote:
Hi,
Why reviewing a patch I got into this error from twistedchecker. This is old code, just that someone it was touched by recent changes,
Also: pre-existing errors like this one should not block new development work being done. It is *not* a requirement that a patch fix all nearby but unrelated problems like this one.
OK. but then maybe twistededchecker and pyflakes should not be part of the stable builders. In my head, all stable builders should pass before merging new code.
New and changed code should conform to the standard and try not to introduce new errors. If the "errors" being introduced are due to bugs in a tool that reports errors, the bugs should be fixed.
New code should only try not to introduce new errors? I was expecting that new code is not allowed to introduce new errors. Period.
Existing code *near* new or changed code can and should be left alone.
Does this mean that The (boy) scout rule does not apply for Twisted development? Thanks! -- Adi Roiban
On 02:09 pm, adi@roiban.ro wrote:
On 25 January 2015 at 01:01, <exarkun@twistedmatrix.com> wrote:
On 24 Jan, 07:51 pm, adi@roiban.ro wrote:
On 24 January 2015 at 19:23, <exarkun@twistedmatrix.com> wrote:
On 06:45 pm, adi@roiban.ro wrote:
Hi,
Why reviewing a patch I got into this error from twistedchecker. This is old code, just that someone it was touched by recent changes,
Also: pre-existing errors like this one should not block new development work being done. It is *not* a requirement that a patch fix all nearby but unrelated problems like this one.
OK. but then maybe twistededchecker and pyflakes should not be part of the stable builders. In my head, all stable builders should pass before merging new code.
New and changed code should conform to the standard and try not to introduce new errors. If the "errors" being introduced are due to bugs in a tool that reports errors, the bugs should be fixed.
New code should only try not to introduce new errors?
The tools are there to help development, not hinder it. If a tool is producing an error that doesn't help development, slavishly adhering to a policy that requires additional work that doesn't help the project is counter-productive. If all the tools were perfect we could say that all new code *must* never introduce new errors. Unfortunately the tools are not perfect.
I was expecting that new code is not allowed to introduce new errors. Period.
Existing code *near* new or changed code can and should be left alone.
Does this mean that The (boy) scout rule does not apply for Twisted development?
I'm not familiar with "The (boy) scout rule". Jean-Paul
On Jan 25, 2015, at 7:21 AM, exarkun@twistedmatrix.com wrote:
The tools are there to help development, not hinder it. If a tool is producing an error that doesn't help development, slavishly adhering to a policy that requires additional work that doesn't help the project is counter-productive.
I think the consensus rule we are converging on for this is, if you notice a bug in the tool, you have to report the bug in the tool. If the bug already exists, just link to the issue which is affected by that bug, but if it's actually a bug and not just a coding-standard thing you don't feel like fixing, you should not block development on it. This seems like the right way to proceed with spurious warnings so we don't add tons of pointless busy-work but we also don't just develop a culture of only a special inner circle knowing which warnings are OK to ignore.
Does this mean that The (boy) scout rule does not apply for Twisted development?
I'm not familiar with "The (boy) scout rule".
I believe that Adi is referring to <http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule <http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule>>, "Always leave the campground cleaner than you found it." While every change should be an improvement, I think the Burning Man creed of "no matter out of place" would be a better way to formulate the policy :). You should clean up the things that you're changing, but every unrelated change is an extra burden on the reviewer, so it's not doing anyone any favors to make patches super long to fix every trace of coding standard violation. Having dedicated cleanup branches is fine, but they should also be finite in their scope, and not clean up things beyond the specific area that they're trying to fix. -glyph
participants (3)
-
Adi Roiban -
exarkun＠twistedmatrix.com -
Glyph