Enable black formatter in CI #602

karlding · 2019-05-21T04:41:37Z

Here's the second part that enables black formatter checks on the project.

The black formatter returns 0 if nothing would change, and 1 if files would be reformatted, which allows the CI job to fail.

black --check .

In order to provide some more verbose error reporting, we also run the following to provide the diff and show the modifications (although this isn't strictly necessary).

black --diff .

I've split the commits for easier review so that the final commit is purely the results of running

black .

Disable the pylint warning for the block, as black will reformat this over multiple lines instead of the previous one-liner.

karlding · 2019-05-21T04:42:10Z

Also, I wrote this quick and dirty bash script to dump all the Python files that are currently touched by open PRs.

ACTIVE_PRS=(290 539 554 567)
DIFF_FILE="output.txt"

echo "" >| "${DIFF_FILE}"

for PR in "${ACTIVE_PRS[@]}"; do
  git fetch origin pull/"${PR}"/head:pr-"${PR}"
  git diff --name-only develop...pr-"${PR}" | grep '.py' | tee -a "${DIFF_FILE}"
done

sort "${DIFF_FILE}" -o "${DIFF_FILE}" -u

I looked at those PRs, and the changes don't seem too bad in context of this giant format patch. Otherwise, if we want to, we can simply exclude these files until those PRs are merged.

can/__init__.py
can/interface.py
can/interfaces/__init__.py
can/interfaces/pcan/pcan.py
can/interfaces/serial/__init__.py
can/interfaces/serial/serial_can.py
can/interfaces/serial/simpleserial.py
can/interfaces/serial/slcan.py
can/interfaces/slcan.py
can/interfaces/usb_can_analyzer/__init__.py
can/interfaces/usb_can_analyzer/usb_can_analyzer.py
can/io/__init__.py
can/io/mf4.py
examples/receive_all.py
examples/serial_com.py
setup.py
test/interface_simpleserial_test.py
test/interface_slcan_test.py
test/interface_test.py
test/logformats_test.py
test/serial_test.py

codecov · 2019-05-21T04:51:02Z

Codecov Report

Merging #602 into develop will decrease coverage by 0.1%.
The diff coverage is 69.42%.

@@             Coverage Diff             @@
##           develop     #602      +/-   ##
===========================================
- Coverage    63.79%   63.68%   -0.11%     
===========================================
  Files           63       63              
  Lines         5532     5541       +9     
===========================================
  Hits          3529     3529              
- Misses        2003     2012       +9

codecov · 2019-05-21T04:51:02Z

Codecov Report

Merging #602 into develop will decrease coverage by 0.12%.
The diff coverage is 69.29%.

@@             Coverage Diff             @@
##           develop     #602      +/-   ##
===========================================
- Coverage    63.79%   63.66%   -0.13%     
===========================================
  Files           63       63              
  Lines         5532     5543      +11     
===========================================
  Hits          3529     3529              
- Misses        2003     2014      +11

felixdivo · 2019-05-21T12:12:47Z

can/interfaces/__init__.py

-    'slcan':            ('can.interfaces.slcan',            'slcanBus'),
-    'canalystii':       ('can.interfaces.canalystii',       'CANalystIIBus'),
-    'systec':           ('can.interfaces.systec',           'UcanBus')
+    "kvaser": ("can.interfaces.kvaser", "KvaserBus"),


Do we really want this new formatting here? Or is there no option to exclude a block?

black provides a way to exclude sections from formatting, but IMO that kind of goes against the point of having a formatter.

If someone feels strongly about keeping it untouched by the formatter, I can add

# fmt: off # ... # fmt: on

felixdivo · 2019-05-21T12:14:32Z

can/interfaces/canalystii.py


        for channel in self.channels:
-            if CANalystII.VCI_InitCAN(VCI_USBCAN2, self.device, channel, byref(self.init_config)) == STATUS_ERR:
+            if (


Maybe:

Suggested change

if (

status = CANalystII.VCI_InitCAN(VCI_USBCAN2, self.device, channel, byref(self.init_config))

if status == == STATUS_ERR:

felixdivo · 2019-05-21T12:15:07Z

can/interfaces/canalystii.py

        timeout = -1 if timeout is None else int(timeout * 1000)

-        if CANalystII.VCI_Receive(VCI_USBCAN2, self.device, self.channels[0], byref(raw_message), 1, timeout) <= STATUS_ERR:
+        if (


split into two statements?

felixdivo

Well, I don't really like the style, but that is kind of subjective so I'm fine with it if you want it since it at least is consistent.

Maybe it would be nice to exclude some code "blocks", like in can/interfaces/__init__.py or large parts of can/interfaces/ixxat/constants.py. I don't know whether that's possible or desired.

felixdivo · 2019-05-21T12:21:12Z

I didn't go through all of the changes, since it was just tom much.

felixdivo · 2019-05-21T12:22:45Z

We could also add the badge: https://github.com/python/black#show-your-style

felixdivo · 2019-05-21T12:24:34Z

I did really like the previous semantics of quotes: Double quotes for user facing text, single quotes for everything else. But I guess that's a matter of taste as well.

felixdivo · 2019-05-21T12:28:59Z

.travis.yml

+        - travis_retry pip install -r requirements-lint.txt
+      script:
+        - black --diff .
+        - black --check .


I could image black --check --verbose . being enough, since black itself uses it

Clean up formatting of the CANalystII interface after running Black formatter.

Switch to using: black --check --verbose . Providing the diff isn't too valuable when the formatting is automatically handled by the formatter.

karlding added 4 commits May 20, 2019 20:58

Add black to requirements-lint.txt

e5cfee2

Enable black in Travis CI checks

91da45e

Fix pylint directive comment

21620c2

Disable the pylint warning for the block, as black will reformat this over multiple lines instead of the previous one-liner.

Format files using black formatter

5b42f83

karlding requested review from felixdivo and hardbyte May 21, 2019 04:41

felixdivo reviewed May 21, 2019

View reviewed changes

felixdivo added the QA about improving and maintaining the quality of the library label May 21, 2019

felixdivo reviewed May 21, 2019

View reviewed changes

felixdivo assigned karlding May 21, 2019

karlding added 2 commits May 23, 2019 22:10

Clean up CANalystII interface formatting

b15319e

Clean up formatting of the CANalystII interface after running Black formatter.

Change CI command

902a13c

Switch to using: black --check --verbose . Providing the diff isn't too valuable when the formatting is automatically handled by the formatter.

hardbyte approved these changes May 27, 2019

View reviewed changes

hardbyte added this to the 4.0 Release milestone May 27, 2019

hardbyte merged commit c79bba8 into hardbyte:develop May 27, 2019

felixdivo mentioned this pull request Jun 3, 2019

Adding code formatter/linter to workflow? #579

Closed

karlding deleted the enable_black_formatter branch July 11, 2019 03:15

	if (
	status = CANalystII.VCI_InitCAN(VCI_USBCAN2, self.device, channel, byref(self.init_config))
	if status == == STATUS_ERR:

Enable black formatter in CI #602

Enable black formatter in CI #602

Uh oh!

Conversation

karlding commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karlding commented May 21, 2019

Uh oh!

codecov bot commented May 21, 2019

Codecov Report

Uh oh!

codecov bot commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

felixdivo May 21, 2019

Choose a reason for hiding this comment

Uh oh!

karlding May 24, 2019

Choose a reason for hiding this comment

Uh oh!

felixdivo May 21, 2019

Choose a reason for hiding this comment

Uh oh!

felixdivo May 21, 2019

Choose a reason for hiding this comment

Uh oh!

felixdivo left a comment

Choose a reason for hiding this comment

Uh oh!

felixdivo commented May 21, 2019

Uh oh!

felixdivo commented May 21, 2019

Uh oh!

felixdivo commented May 21, 2019

Uh oh!

felixdivo May 21, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

karlding commented May 21, 2019 •

edited

Loading

codecov bot commented May 21, 2019 •

edited

Loading