PEP8 compliance in pydot.py

[hrishikeshrt]

No description provided.

[peternowee]

Hi @hrishikeshrt, thanks for this PR. The timing is great, at the start of a new development cycle.

I think we should be able to merge this soon. There is only one thing I really have strong doubts about:

• These separator lines:

  # --------------------------------------------------------------------------- #
  

  Is there any style guide or style checker that recommends this practice or is it just a personal preference? I can see their use, but personally I doubt I would depend much on them when moving through the code. More importantly, if there is no standard to enforce them, then with multiple contributors working on the code I am afraid their usage will become inconsistent over time. I also do not want to introduce a project-specific style rule for this. So unless there is a style guide or linter that this came from that you recommend, I would prefer to leave them out.

My other remarks are optional. You can decide per item what you want to do with it. They are not blocking merging as far as I am concerned:

• From the remaining flake8 complaints: pydot.py:110:80: E501 line too long (82 > 79 characters). Any reason not to add a line break here? Maybe the 10% rule used by Black and Bugbear B950?
• From the remaining flake8 complaints: pydot.py:393:5: F841 local variable 'node_orig' is assigned to but never used. That variable was probably left over after copy-pasting from the method above it. If you want, you can remove it.
• Commented-out code # for attr in self.obj_dict['attributes']: can actually be deleted. It was introduced by commit 842173ca of 2008 (pydot 1.0.2) and it looks like it was an early draft still based on the idea that obj_dict['attributes'] would contain all possible Graphviz attributes, just as the attributes property did before pydot 1.0.2. However, in the end the lists of Graphviz attributes were moved to constants to be passed to obj_attributes. Anyway, I do not think anybody still needs that comment. If you want, you can delete it.
• Commented-out code # self.node_dict[graph_node.get_name()] = graph_node.attributes can be deleted as well. It was also introduced by commit 842173ca of 2008 (pydot 1.0.2). The commented-out line is clearly outdated, because I do not see when node_dict was ever in use and .attributes was removed in that same commit. It seems more like a very early draft for the line above it than a hint or alternative for future readers.
• This commented-out code can also be deleted:

  #match.extend( Subgraph( obj_dict = obj_d )
  #             for obj_d in obj_dict_list )
  

  Introduced by commit b125c5d7 of 2010 (pydot 1.0.3). Also seems to have been an early draft. Not sure if it would even work.
• test/pydot_unittest.py not done yet. Not meaning to ask you to spend more time than you have. Your work is already greatly appreciated.
• In the end I would prefer to squash everything into one single commit. If you want, you can do that already and force-push over this PR branch. I can also do it on merging.

Side notes, not requiring change to your proposal:

• AppVeyor CI failure not related to the changes of this PR, but due to known issue Inconsistent GraphViz output on AppVeyor/Windows/Anaconda #213. Travis CI passes.

• From the remaining flake8 complaints:

  pydot.py:32:16: F821 undefined name 'basestring'
  pydot.py:308:13: F821 undefined name 'unicode'
  pydot.py:1688:21: F821 undefined name 'unicode'
  dot_parser.py:34:16: F821 undefined name 'basestring'
  

  Related to Python 2 and safely behind a conditional checking for PY3. Indeed keep them for now as proposed. Will be removed later, after Drop Python 2 and 3.4 support #229.

• From the remaining flake8 complaints:

  dot_parser.py:407:9: F841 local variable 'dot' is assigned to but never used
  dot_parser.py:408:9: F841 local variable 'slash' is assigned to but never used
  dot_parser.py:409:9: F841 local variable 'bslash' is assigned to but never used
  dot_parser.py:410:9: F841 local variable 'star' is assigned to but never used
  

  These unused local variables are part of a longer list of variables, most of which actually do get used. Indeed, it is probably better to just leave the few unused ones around as well, in case we ever need them in the future. So, no need for changes to your proposal.

• In graph_definition() and quote_if_necessary(), some vertical alignment gets lost, making them slightly less readable. But indeed, PEP 8 Pet Peeves recommends avoiding this. Pros and cons can be found here:

  • https://stackoverflow.com/questions/13497793/python-alignment-of-assignments-style
  • https://stackoverflow.com/questions/4001375/is-it-considered-a-good-style-to-use-whitespace-to-align-the-code

  I have no problem with giving up the alignment here, mainly because readability is not hurt too much and it prevents complaints from code checkers, which is also burden. Again, your proposed changes here are ok for me.

• The commented-out code # dblQuotedString and # .setParseAction(strip_quotes) | are also very old, from commit 842173ca of 2008 (pydot 1.0.2). But in this case, we actually might be working on that part of the code soon, so maybe they will come in handy then and can be deleted after that. So, your proposed change is ok as it is.

• Last year, @ankostis suggested letting Black take care of coding style. I tried it out yesterday and it does seem quite easy, though also very opinionated. The initial run will make many changes to our code: Not only PEP 8, but also many other things (longer maximum line length, additional line breaks, additional parentheses, double quotes everywhere, etc.). So, I think we will need some more time to let everyone review the changes brought in by Black. I am thinking of merging your PEP 8 changes soon already and then open a separate PR for the introduction of Black. This has the added benefit of seeing only the non-PEP8 changes that Black introduces. Love to hear your opinion on Black (or alternatives). And if you are interested in preparing that PR as well, please let me know beforehand, so that we are not doing double work.

[hrishikeshrt]

• Yes, the separator lines are a personal preference and a force of habit. I will get rid of those.
• The reason I did not break line 110 was because it had != [] at the end and breaking it was really hampering the readability of the code. I've updated this by splitting it in two lines
• Got rid of node_orig = 1 unused variable
• Got rid of several comments that you have mentioned
• For the unused variables which may be used later, I've added #noqa to suppress the current complaints.
• Completed pydot_unittest.py as well (with exception of two unused graph instances, which I suspected might be part of the test case to not raise error.
• Regarding Black, I have no personal experience, and if it means going away from PEP8, then that's a negative for me. However, I will need to check that out before giving an informed opinion though. (I am open to submitting that PR as well if we end up deciding on using Black).

[ankostis]

Have you considered black instead of/in addition to flake?

[prmtl]

I would also suggest to just use black (+ integrate it in CI) and forget about formatting ;)

[peternowee]

@hrishikeshrt Thanks for all the changes. A few more remarks:

• Yes, the separator lines are a personal preference and a force of habit. I will get rid of those.

Ok, thanks, but now there are still 7 left in pydot.py. They have only been removed from dot_parser.py.

• The reason I did not break line 110 was because it had != [] at the end and breaking it was really hampering the readability of the code. I've updated this by splitting it in two lines

Ok, yes, I see what you mean. Nicely solved.

• For the unused variables which may be used later, I've added #noqa to suppress the current complaints.

Hmm. Not really necessary for me actually. Some considerations:

• Without a specific error code, # noqa will hide other problems as well. Coincidentally, there is now a E261 error hiding in line 397 already (E261 at least two spaces before inline comment). So # noqa: F841 (and an extra space) would be better, but...
• Ignoring it in pylint as well would require another pragma again, and the pylint one needs to come first: # pylint: disable=unused-variable # noqa: F841. That makes it even longer, so...
• It would be nice if we could apply the rules to the entire block instead of just those 4 lines, but ignoring a block of lines is not possible in flake8 right now. (It is possible in pylint by toggling the rule using # pylint: disable=unused-variable, # pylint: enable=unused-variable.)
• More importantly, we do not have any other pragmas for flake8 or pylint in the code right now and I am still not sure if we should start that practice at all. I have been considering a pylint pragma as part of PR pydot/pydot#242, but also still not sure about it. They could make the code quite messy in the long term while perhaps not so many people actually run these tools on our code.

What do you think? If it is all the same to you, maybe better remove them for now.

• Completed pydot_unittest.py as well (with exception of two unused graph instances, which I suspected might be part of the test case to not raise error.

I see flake8 report only one (not two) unused graph instance, in line 44, in test_add_style(). Although I think it can be removed, I agree that such a change does not really belong in a PEP8 commit. So, I agree that it is better to leave that line in for now, as you propose.

The second issue I see flake8 report is about a missing space:

pydot_unittest.py:65:12: E225 missing whitespace around operator

From line 65:

d ='digraph {\na -> b[label="hi", decorate];\n}'

Hope you can still correct that.

For the rest, all your changes to test/pydot_unittest.py look good to me.

One new remark, something that I did not notice last time, sorry. Optional:

• I see that in many places you remove white space between the def lines and the first lines of code, but not everywhere. A clear example is here: Still an empty line below def push_graph_stmt(), while the one below def push_subgraph_stmt() was removed. Also in a few other places in dot_parser.py and test/pydot_unittest.py.

Separate comment on black coming up tomorrow, but that should not affect this PR.

[hrishikeshrt]

• Removed the leftover separator lines in pydot.py, sorry about missing them earlier
• Removed # noqa
• I actually missed the space around operator error and thought it's an error about graph declaration on the next line (that's also why I mentioned there are two unused graph variables). Corrected.
• I've removed the blank lines after def. The ones left over were not intentional but from an oversight.

Thank you.

[peternowee]

Wow, you're faster than I can turn off my computer. 😄

But you still missed a few blank lines after def lines, including the one I mentioned. Anyway, like I said this is optional, so you decide if you still want to make a final correction or not and then I want to move on to merging.

Do you want to squash your commits yourself or shall I do it on merging?

[hrishikeshrt]

I think I've got them all this time 😅
You may go ahead with the merge.

I've also squashed commits (I believe).
(In my repository I only see a single commit, although all of them show up here, so I am not sure if it was done properly.)

[peternowee]

Yes, but now you also removed empty lines below a few class lines (or their docstring). Those actually should have an empty line below them. See https://stackoverflow.com/questions/18914108/python-pep-blank-line-after-function-definition/34818759#34818759 and https://www.python.org/dev/peps/pep-0257/.

Also, you removed empty lines in other parts of the code, while this was supposed to be a final correction.

Could you please just revert from the last commit before squash (17f4a35) all changes that went beyond what I requested in my last comment (only remove the blank lines after def)?

[ankostis]

* This has the added benefit of seeing only the non-PEP8 changes that Black introduces. Love to hear your opinion on Black (or alternatives).

Yes, indeed.
But you'll see, once black, never back :-)

[peternowee]

@ankostis Yes, maybe you are right, but I also still have a few questions/doubts about black. Will try to post them here soon.

[ankostis]

Regarding the longer line-width, there been much debate, sprung from Hettinger's "don't PEP8-me" notorious presentation, and it does make sense in multiple ways, e.g. we don't want a slight change to produce a much bigger git-diff.

In general black has been streamlined for git and cooperating developers, mimicking the success of Golang's automated formatting. The whole point is to avoid having lengthy discussions like the one above.

[hrishikeshrt]

Could you please just revert from the last commit before squash (17f4a35) all changes that went beyond what I requested in my last comment (only remove the blank lines after def)?

Done. Also, There were some instances where class instances did not have a blank line after them. Added that as well.
Please take a look.
I have not squashed again. If this version is okay, you may squash post merging (unless you want me to squash).

[peternowee]

@hrishikeshrt
Yes, class headers look much better now, thank you for picking that up as well.

Empty lines below def not completely consistent yet. Your last correction even brought back some of the empty lines you had correctly removed in 17f4a35, but compared to master most are gone now and no new ones were introduced, so in total a huge improvement.

Overall, a lot of inconsistencies removed, completely PEP 8 compliant now and everything looking great again. So, finally merged. Thanks a lot for all your hard work!

I will probably need another day to write down my thoughts on black.

[peternowee]

@ankostis wrote:

Have you considered black instead of/in addition to flake?
[...] you'll see, once black, never back :-)

@prmtl wrote:

I would also suggest to just use black (+ integrate it in CI) and forget about formatting ;)

As you may have noticed from my delayed reply, I have been struggling with this for days now already. Especially because the two of you are so positive about it, while I am not overly enthusiastic yet. I can see the promise of automated code formatting, but I do not know if it is going to be so enjoyable to work under it. It takes away part of the art, the fun. There is no leniency at all. If the result is ugly, just suffer it. Computer says no. Another part of our lives controlled by a bot.

And not even a very good bot, I feel. I have also been looking at yapf and it seems a bit smarter, or at least it aspires to be, with its penalty scoring approach. Also, it seems the yapf developers are more open to improving their tool to produce the most optimal code formatting for humans to work with, aware that that bar is very high and has not been reached yet. With black I get the impression that they fixed the bar a few years ago and now the goal is just to keep producing the same result forever. I looked up some of the issues I had with black on their issue tracker and the answer always seems to be like: "No, sorry. Remember our slogan: Uncompromising.". But while I can understand you want a code formatter to be uncompromising at the moment you run a certain version of it, I don't need it to be uncompromising between versions. They could at least try to improve over time.

A non-technical problem I have with yapf though is that it is led by Google, meaning a Contributor License Agreement (CLA) for contributors and uncertainty about its future in case Google suddenly loses interest in it. In contrast, black is under the Python Software Foundation umbrella.

Then there is autopep8, which I also tried out a bit. Given that we just had a PEP8 refresh, it does not make many changes. It has a much more limited reach than black and yapf, meaning it leaves a lot of formatting untouched. But in the areas that it covers, the choices it makes are the same as the choices black and yapf make. So, it could be used as a first step into automated code formatting. A problem with autopep8 though is that it is a much smaller project and that it needs some strange command line options like -aa --experimental (-a meaning --aggressive). An issue to make those flags the default has been open for a few years already. Also, I do not know how well it can integrate with CI.

@ankostis wrote:

Regarding the longer line-width, there been much debate, sprung from Hettinger's "don't PEP8-me" notorious presentation, and it does make sense in multiple ways, e.g. we don't want a slight change to produce a much bigger git-diff.

Yeah, ok, well, I watched that Raymond Hettinger talk, but I am not sure if black catches the full meaning of it. Ok, it raises the line-width from 79 to 88, but in enforcing that black seems to be just as strict as someone who strictly enforces the 79 limit of PEP 8. The documentation mentions the 90-ish characters, the 10% tolerance, so my first impression was that 88 was the result of a 80 character limit plus 10% tolerance, that sounded reasonable. But in reality, 88 seems to be just the new limit and there is no tolerance. If you have a multi-line collection of items and try to stay within 80 characters, because you do not want to use that 10% tolerance there, black will still just reflow your code to fill up lines to the maximum of 88. And if you need that 1 extra character and use 89, Black will reformat to 88 or lower again. So where is that tolerance, where is that "...-ish"? Maybe I am missing it, so please correct me if I am wrong. Otherwise it seems black simply applies its rules just like a PEP 8 zealot applies PEP 8: Like a law book. Which is exactly what Raymond Hettinger in that talk says not to do.

In general black has been streamlined for git and cooperating developers, mimicking the success of Golang's automated formatting. The whole point is to avoid having lengthy discussions like the one above.

Yes, it is easy to point out a lengthy discussion about code formatting style, but I think we do not have a lot of them actually. The last PEP8 refresh was in 2016. What is more difficult to point out is the costs of using a code formatter: How do you measure the cost of everyone having to scroll through 170 additional lines of code at the top of pydot.py, the cost of everyone having to ignore inconsistent formatting of similar lines because of slightly different line lengths, etc.

Also, how about newcomers? Is this not a slight entry barrier? Predicting a formatting tool is even more difficult than following PEP 8. So we would need to ask new contributors to install that tool (maybe even pip still), or else let them hit a failed CI check when they submit their PR. Does the CI check use the black --diff option so they can see what they are supposed to change? I assume we do not want a CI bot to make formatting commits by itself. I tried to find some examples of projects using black CI integration to see how this works out in reality, but did not find any yet. For example, contributor submits PR, check fails, maybe some discussion, second try, check succeeds, PR gets merged. If someone has some examples like that, please let me know, so I can get a feel for how this works in practice.

There is so much more to say, but I will leave it at this now. Hope to hear your thoughts. I am not all negative about it, just struggling with it.

Some comparisons:

• master vs. black
• master vs. yapf-facebook
• master vs. yapf-google
• master vs. yapf-pep8
• master vs. autopep8
• black vs. yapf-facebook

(I tweaked the settings a bit to make them easier to compare. For details, see the individual commits of each PR.)

[prmtl]

I agree that black is not a tool that fits all. But actually this discussion would not happen if we would have black (or any opinionated formatter in that matter) ;) The point is - use it and forget, do not think about style anymore, do not think if better to use facebook style, google style or whatever. It will be always the same for everyone and (virtually) forever thanks to:

"No, sorry. Remember our slogan: Uncompromising."

Also, how about newcomers? Is this not a slight entry barrier?
This is just like with regular tests, if they fail - it need to be fixed. Bugfix? Please provide example.

And how it works now with PEP8? They need to (should) follow it as to not reject contribution due to formatting or badly formatted code will be introduced into the codebase. Even more: if they fail to stick to formatting rules, it's easier to just run black command by maintainer of pydot on the proposed change than to fix formatting manually.

For me it could be any formatter as long as you don't have to think when using it. Sadly with yapf you sometimes do, to just choose the options or preset.

IMHO lets not overthink it (as pydot is rather small codebase) and just integrate black on defaults or even yapf with one of the predefined config and stop worrying about formatting.

[ankostis]

And if you need that 1 extra character and use 89, Black will reformat to 88 or lower again. So where is that tolerance, where is that "...-ish"?

As explained, the limit is 80 +8(%10), but in that style-manual they also suggest you to make it 90-ish, which btw, the line-length is one of the few configurable parameters of black.

Also you can always mark parts of the code to be excluded from the black, with #fmt: on/off,

Also, how about newcomers? Is this not a slight entry barrier?

Quite the contrary, black is a gift - to me it's always a delight to see the black-badge at the top of a project's coordinates:

As long as you have ignored any filename-regexes that must no autoformat, with a section in pyproject.toml like this:

[tool.black]
exclude = <regex>

all a contributor has to do it to type black . to format his code.

So we would need to ask new contributors to install that tool (maybe even pip still), ...

Actually it is a good habit to provide an extras [dev] in setup.py (or/and [all]) so all such dependencies are installed:

    extras_require={
        "dev": "black==21.5b2; python_version > '3.5'",
        ...

Note that the pinning is good for 2 reasons (1, to be stable, 2. it has no stable release, and causes dependency problem sometimes)

I tried to find some examples of projects using black CI integration to see how this works out in reality, but did not find any yet.

I have contributed to programs that have a pre-commit hook for black`](https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/), but the experience wasn't that great.

If someone has some examples like that, please let me know, so I can get a feel for how this works in practice.

Eventually having a test-case that simply runs black --check . or black --diff . demanding an ok exit-code, would print the user all unformatted files.

Look also the github-actions example.
I thought i had wrote something similar in travis, but cannot find it now.

The eventual experience as a contributor is rather amusing, you just curse yourself for not having run the test-cases locally prior to push, fix, push-force to re-write commit, and you get accustomed in no time.

---

Thank you for your always dutiful collection of resources, for the list of comparisons.

But I'm surprised you didn't already mention the reformatting of string-quotes to " - that is a first big surprise to newcomers to black. It's only later that you come to appreciate its significant side-effect: you can immediately tell if some code has been black or not (yet).

In any case, as @ prmtl pointed out, the point is to have an opinionated auto-formatter, whatever that might be.

[peternowee]

@prmtl @ankostis If we were to give black a try, would the following 4 choices be ok for both of you? These would solve my biggest gripes:

• line-length = 79: A personal preference, because I often work on a small laptop or even my phone. And when I work on an external monitor, I often do side-by-side diffs and 3-way diffs.
• skip-string-normalization = true: This prevents conversion of single quotes to double quotes. In case a better tool comes around in a few years that does not mandate double quotes. We can always switch to double quotes later if it really becomes the norm. Not a deal breaker for me, though.
• Wrap the *_ATTRIBUTES constants at the top of pydot.py in the # fmt: off/on pragmas suggested by @ankostis to prevent they get exploded to a single item per line. I would rather have a larger diff of 60 lines once every few years than having 170 additional lines of code around all the time.*
• No pressure on contributors to install the formatting tool locally or to ensure their code passes the formatter before they submit a PR. This affects how we document it (a badge is ok of course), but also for example a formatting check in the test suite should not fail if the formatter tool (e.g. black) is not installed. I would not want to miss someone sharing a quick bug fix. The formatter check can be part of the CI checks on GitHub after the PR is submitted, preferably listed separate from the regular test suite**. In case of formatting issues, we can always suggest they fix them, but if that is not possible (e.g. we responded too late and the contributor has moved on to do other things), we can also just merge their work and fix the formatting ourselves in a subsequent commit.

* I guess the exploding of these long lists is what got black and me off on the wrong foot: It was the first thing I saw after running black for the first time. I really had to get past that to discover that the rest was not so bad. There is some discussion on this behavior (psf/black#1345, psf/black#1824, psf/black#1288, etc.), btw, but considering discussions like psf/black#1252 I am not getting my hopes up.

** Also the formatting check needs run only once, no need for running it on multiple architectures and Python versions.

With regard to yapf, I went through the comparison of black vs. yapf (facebook style) of peternowee#7 and I cannot say one is clearly better than the other. In some areas, yapf can be told to behave like black by adding trailing commas or through configuration, but some things are simply not possible in yapf yet, such as adding/removing parentheses or listing multiple logical operators line by line. Forcing ourselves to stay with one of the predefined styles we would lose out on one of the advantages of yapf actually, but on the other hand I agree the time needed to agree on it all would simply not be worth it. So, no clear winner and if you guys prefer black, I am fine with that.

With regard to autopep8: If the premise is that less formatting/style choices is better, I guess the wider reach of black and yapf should be preferred over the limited reach of autopep8.

In reply to @ankostis

And if you need that 1 extra character and use 89, Black will reformat to 88 or lower again. So where is that tolerance, where is that "...-ish"?

As explained, the limit is 80 +8(%10), but in that style-manual they also suggest you to make it 90-ish, which btw, the line-length is one of the few configurable parameters of black.

The link is not working and I am not sure if you understood what I meant, namely that black (let's assume the default of 88 characters) does not treat the "+10% tolerance zone" (between 80 and 88) any differently than the "100%" zone (between 0 and 80 characters). AFAICT, it does not feel any restriction to use the full 88 characters when reflowing. And it is also not like 88 has become the new 100% with a 10% tolerance on top of that, because anything above 88 is treated as a violation. But I think the documentation already confirms my suspicion by using the word "happens" here:

Black defaults to 88 characters per line, which happens to be 10% over 80.
-- https://github.com/psf/black/blob/21.5b2/docs/the_black_code_style/current_style.md#line-length

I think a true implementation of a tolerance of 10% would be if the tool would only seldomly reflow lines to end inside that tolerance zone. yapf has an option split_penalty_excess_character that seems to be more like what I meant, although I did not have a chance to experiment with it yet.

Note that this is something else than the tool being "uncompromising" or not. A tool can have sophisticated rules on how to behave in the 10% tolerance zone and still enforce them in an uncompromising way.

Also, how about newcomers? Is this not a slight entry barrier?

Quite the contrary, black is a gift - to me it's always a delight to see the black-badge at the top of a project's coordinates:

Yes, ok, to you it is a gift, because you already know the tool and have it installed. I was thinking more about people sharing one of their first Python bug fixes ever.

Anyway, as I stated above, I am ok with it now as long as we do not force contributors to use it before submitting their work.

As long as you have ignored any filename-regexes that must no autoformat, with a section in pyproject.toml like this:

[tool.black]
exclude = <regex>

all a contributor has to do it to type black . to format his code.

Yep, that config file will need to be added. Not seeing any files that need to be excluded at the moment, but the other options I mentioned above go in that same file.

So we would need to ask new contributors to install that tool (maybe even pip still), ...

Actually it is a good habit to provide an extras [dev] in setup.py (or/and [all]) so all such dependencies are installed:

    extras_require={
        "dev": "black==21.5b2; python_version > '3.5'",
        ...

Note that the pinning is good for 2 reasons (1, to be stable, 2. it has no stable release, and causes dependency problem sometimes)

Yes, I guess version pinning is good to ensure the same outcome locally as on CI, but does this not lead to conflicts with other packages pinning on different versions? Or are these requirements installed inside the project directory? I do not have much experience with installing through setup.py.

Also, would it not give us the extra work of watching black updates and manually updating the pinned version regularly/in case of important bug fixes?

I tried to find some examples of projects using black CI integration to see how this works out in reality, but did not find any yet.

I have contributed to programs that have a pre-commit hook for black`](https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/), but the experience wasn't that great.

If someone has some examples like that, please let me know, so I can get a feel for how this works in practice.

Eventually having a test-case that simply runs black --check . or black --diff . demanding an ok exit-code, would print the user all unformatted files.

Look also the github-actions example.
I thought i had wrote something similar in travis, but cannot find it now.

Thanks, I will check out the documentation. If you still find something later, please share. I can also search a bit more myself. Just a random search on GitHub already yields many results, but it seems like every project has a different approach and many of them are too small to have any example PRs I can look at. Anyway, the CI integration can be done separate from the reformatting and documentation, so I am not too worried about that.

The eventual experience as a contributor is rather amusing, you just curse yourself for not having run the test-cases locally prior to push, fix, push-force to re-write commit, and you get accustomed in no time.

Ok, cool. Thanks for taking away that worry.

But I'm surprised you didn't already mention the reformatting of string-quotes to " - that is a first big surprise to newcomers to black. It's only later that you come to appreciate its significant side-effect: you can immediately tell if some code has been black or not (yet).

Yes, it was one of the main things I did not like, but like I said "there is so much to say". I had been struggling with the whole idea of even handing out control to any formatting tool, and got blocked by the overchoice of tools and settings, all new to me. Time was ticking away, so I decided it was better to give a short response rather than none at all. The double quotes were not on the top of my mind that day. As written above, I do hope that we can start with not reformatting the quotes, but it is not a deal breaker for me.

In any case, as @ prmtl pointed out, the point is to have an opinionated auto-formatter, whatever that might be.

Ok, your messages are clear and understood. I am starting to get interested in trying it out. Hope you can agree with the proposal at the top of this comment.

In reply to @prmtl

Also, how about newcomers? Is this not a slight entry barrier?

This is just like with regular tests, if they fail - it need to be fixed. Bugfix? Please provide example.

And how it works now with PEP8? They need to (should) follow it as to not reject contribution due to formatting or badly formatted code will be introduced into the codebase. Even more: if they fail to stick to formatting rules, it's easier to just run black command by maintainer of pydot on the proposed change than to fix formatting manually.

Yeah, ok, but the difference is that now the humans can still make an exception. For example, it sometimes happens that some code blocks are similar to each other, many examples of that in pydot_unittest.py. By formatting them the same, it is easy to spot where they differ, so what they test. The formatter tool will just look at line length and reflow a few of these blocks and others not, so it becomes much more difficult to spot the similarities and the differences between the blocks. We cannot make any exceptions, because the tool will just keep reverting it. And we also would not want to use pragmas in too many places, because it creates a mess and also excludes the ignored blocks from other formatting.

Anyway, I have come to accept this as a trade-off and as said above, I am willing to try it out now. I do however hope you can accept my proposal not to use the defaults, especially for line length. Please still let me know what you think of my proposal at the top of this comment.

In reply to @hrishikeshrt

Regarding Black, I have no personal experience, and if it means going away from PEP8, then that's a negative for me.

No, I think it still adheres to PEP8. Maximum line length is actually configurable and PEP8 allows maximum line lengths up to 99 characters in case the team agrees on it. If I run black with line length 79 and then flake8, I only get one additional violation:

pydot.py:985:43: E203 whitespace before ':'

For this line:

b = node_str[node_port_idx + 1 :]

Actually this seems to be a problem with flake8 and pycodestyle rather than with black. See:

• https://github.com/psf/black/blob/21.5b2/docs/the_black_code_style/current_style.md#slices
• Flake8 and PEP8 has different rule for the whitespace around colon in a slice. PyCQA/flake8#257
• E203: False positive "whitespace before ':' " on list slice. PyCQA/pycodestyle#373
• Fix false alarm in E203 rule (issue #373, black incompatibility) PyCQA/pycodestyle#914

So, using black would not mean going away from PEP8. It just makes a lot of other changes that go beyond the scope of PEP8.

However, I will need to check that out before giving an informed opinion though. (I am open to submitting that PR as well if we end up deciding on using Black).

I do not know if you have been working on this more? Any thoughts on the whole discussion? I have gotten to know black a bit now, so I could also create a PR myself probably, but due to personal circumstances I am not sure if I can see it through to the end. If you do it, I would still have a lot of suggestions based on my experiments of the last few days, so that would not make it much easier, but maybe the end result will be better with two pairs of eyes on it. Let me know what you prefer.

[hrishikeshrt]

I've been passively following the discussion. I can see the appeal in "never having to format the code" ourselves, sure, but I am not entirely in agreement of "write code and forget about formatting". To elaborate, if one writes code in a certain way (which is not horrible, say mostly PEP8 compliant), Black changes it according to its opinions and one comes back to write something again and doesn't see "familiar" code, it can be a bit off-putting. We can only "forget" about formatting to an extent, since the whole point of formatting is make it "comfortable" for humans.

That said, I think Black does a decent job (anecdotal evidence, as per some small to moderately large examples I've tried in their online playground). I also prefer the line limit of 79 (or 80).

You may create PR yourself. I'll follow the progress and make comments if I have any. (Unfortunately, I cannot commit to the task at the moment).

PS: Thank you for your detailed review of the pull request from earlier and overall thorough research. I certainly enjoyed the experience.

[peternowee]

Continued in #267 with a PR for Black.

[ankostis]

(i know the issue if line-width has been resolved, but found it relevant whatsoever for any future discussion)

• Linus pushes for wider-than-80-char lines for tools like grep, etc.
• DHH (RubyOnRails founder) also pushes for that.