fix the off by one issues #6

bukzor · 2014-04-18T16:28:41Z

Resolves #5

asottile · 2014-04-18T16:31:20Z

+1 code looks good too.

sigmavirus24 · 2014-04-18T18:38:13Z

Just noticed you sent this. I have #7 which resolves this and other issues. I changed the calculation but in a more obvious way. The issue is that the number of decisions is off by one, not the formula which you're changing.

bukzor · 2014-04-18T18:42:42Z

@sigmavirus24 : I noticed that as well, and updated my branch as such.

The dot-graph of the trivial function is much more sensible now ( f1->stmt1 rather than f1->f1) as a bonus.

The --minimum-threshold option for dot graphing was also of by one as a consequence.
I've made the default to show all graphs, even when I've introduced a bug to make the complexity negative =X

bukzor · 2014-04-18T18:44:55Z

updated the diff

sigmavirus24 · 2014-04-18T21:08:57Z

mccabe.py

@@ -286,7 +286,7 @@ def main(argv):
                    help="output a graphviz dot file", action="store_true")
    opar.add_option("-m", "--min", dest="threshold",
                    help="minimum complexity for output", type="int",
-                    default=2)
+                    default=0)


Why should this be 0?

By default, I want to see a graph of everything.
Previously it was 2 when the minimum was in fact 2 due to bugs.
I've made 0 mean "literally everything", so that I can still graph things out when I've introduced bugs.

What about not making the graph utility rely on the threshold?

The only purpose of the minimum threshold is to limit the output for dot mode.
I believe you're confusing this with flake8's -m/--maximum-threshold, which is unrelated.

@bukzor I'm not confusing it with anything. There are multiple purposes to the minimum option. It determines when errors are printed.

As you like, but my perception of the original intent was to cull very large outputs, and you'd lose that ability.

Then why not set the default to 1 and force your own use of this to always use 0.

I thought the intent was to show everything by default, and the original
author simply hadn't considered (with good reason) negative complexities.

A default of 1 would behave quite similarly, but hide bugs from the output,
which is not a good practice.

On Fri, Apr 18, 2014 at 2:43 PM, Ian Cordasco notifications@github.comwrote:

In mccabe.py:

@@ -286,7 +286,7 @@ def main(argv):
help="output a graphviz dot file", action="store_true")
opar.add_option("-m", "--min", dest="threshold",
help="minimum complexity for output", type="int",

default=2)

default=0)

Then why not set the default to 1 and force your own use of this to always
use 0.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6/files#r11788170
.

Buck Golemon
Cell: (408) 202-3901

The problem with a default of 0 is that when used to actually determine the complexity of a file or piece of code (when not using the graph functionality) you will everything printed to the screen.

As you like, this was meant to be a minor improvement.
The 2 was obviously a holdover from buggier days.

:)

sigmavirus24 · 2014-04-18T23:22:52Z

FYI @bukzor I really want to merge this so if you can address that one concern and rebase it, I'd ❤︎ it. :)

bukzor · 2014-04-22T23:32:17Z

The problem with a default of 0 is that when used to actually determine the complexity of a file or piece of code (when not using the graph functionality) you will everything printed to the screen.

This was the previous behavior as well; the smallest complexity a function could have was 2, and functions with a complexity of 2 were printed.

bukzor · 2014-04-22T23:38:53Z

I've updated the diff as you want.

schlamar · 2014-05-08T14:11:05Z

mccabe.py

@@ -67,6 +67,8 @@ def __init__(self, name, entity, lineno):

    def connect(self, n1, n2):
        self.nodes[n1].append(n2)
+        # Ensure that the destination node is always counted.
+        self.nodes[n2]


This is a no-op?!

It causes an entry to be created in a defaultdict.

Ah, I see :)

However, "explicit is better than implicit", so self.nodes[n2] = list() is IMO much better.

@bukzor it's perfectly obvious to those who understand what self.nodes is. Do not feel pressured to do more than is necessary

This change duplicates what was done by @sigmavirus24 on a previous commit in def complexity(self):.

I proposed in the comment https://github.com/flintwork/mccabe/pull/7/files#diff-c8e207640776e7218af4cd9951eb2fceL84 to adjust slightly the computation to highlight the fact that it is num_nodes which was wrong, not num_edges.

However, "explicit is better than implicit", so self.nodes[n2] = list() is IMO much better.

@schlamar That has different behavior. We're not guaranteed that n2 has no successors.
The explicit version of this line would be:

if n2 not in self.nodes[n2]: self.nodes[n2] = self.nodes.default_factory()

Which is, of course, the behavior of defaultdict.__getitem__, except with worse performance.
If you still like this better than it is currently written, I will change it.

@bukzor what you wrote is fine. Anyone who has read the code will understand the expected behaviour.

If a tree has more than one leaf node, #6 will be correct while #7 will not.

@bukzor it seems that the algorithm used by this mccabe script does not support more than one leaf. The proposal #16 does not change the behavior, it fixes num_nodes/num_edges internal computation.

While that may be true (or may not; it's certainly not obvious), it doesn't change the truth of my assertion:

If a tree has more than one leaf node, #6 will be correct while #7 will not.

It's certainly not impossible that the tree will have more than one leaf after future changes.
This leads me to say that #6 is more correct.

Anyone who has read the code will understand the expected behaviour.

It was not obvious for me (with years of Python experience) and I've never seen this before. So an additional comment like "ensure the key is in the defaultdict" wouldn't harm anyone, right? :)

sigmavirus24 · 2014-05-08T14:33:42Z

@bukzor this is still not in a mergable state. Were you planning on rebasing?

bukzor · 2014-05-08T18:29:47Z

Rebased and passed CI.
I was very glad to see the new tests.

https://travis-ci.org/bukzor/mccabe/builds/24735372

schlamar · 2014-05-08T18:44:48Z

.gitignore

@@ -3,3 +3,5 @@
 *.py[cod]
 .tox
 dist
+# silly vim.
+.*.sw[a-z]


you can use your global gitignore for such files :)

I agree with @schlamar about this. Please revert this addition.

bukzor · 2014-07-16T20:47:34Z

Rebased again.
Removed the .gitignore change.
All issues resolved I think.

bukzor · 2014-07-23T20:01:14Z

All issues resolved I think.

sigmavirus24 · 2014-07-23T20:15:05Z

Thanks for the reminder @bukzor. I'll take another look at this tonight.

sigmavirus24 · 2014-08-07T20:19:11Z

Sorry that it took so long to merge this @bukzor

Thanks for the contribution

bukzor · 2014-08-07T20:36:07Z

cool!

Let me know when we should see an improved behavior from flake8.

sigmavirus24 · 2014-08-07T20:58:35Z

@bukzor I don't have maintainer or owner permissions on the PyPI package so this will have to wait for @florentx to have the time to either grant me such permissions or release the package itself.

bukzor mentioned this pull request Apr 18, 2014

Updates and Bug Fixes #7

Merged

sigmavirus24 reviewed Apr 18, 2014
View reviewed changes

bukzor mentioned this pull request Apr 18, 2014

all tests are broken #8

Closed

schlamar reviewed May 8, 2014
View reviewed changes

florentx mentioned this pull request May 8, 2014

Increase number of nodes instead of decreasing number of edges #16

Closed

schlamar reviewed May 8, 2014
View reviewed changes

fix the off by one issues

c306935

sigmavirus24 merged commit c306935 into PyCQA:master Aug 7, 2014

fix the off by one issues #6

fix the off by one issues #6

Conversation

bukzor commented Apr 18, 2014

asottile commented Apr 18, 2014

sigmavirus24 commented Apr 18, 2014

bukzor commented Apr 18, 2014

bukzor commented Apr 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigmavirus24 commented Apr 18, 2014

bukzor commented Apr 22, 2014

bukzor commented Apr 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigmavirus24 commented May 8, 2014

bukzor commented May 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukzor commented Jul 16, 2014

bukzor commented Jul 23, 2014

sigmavirus24 commented Jul 23, 2014

sigmavirus24 commented Aug 7, 2014

bukzor commented Aug 7, 2014

sigmavirus24 commented Aug 7, 2014