Run black on files in package by tsalo · Pull Request #328 · physiopy/phys2bids

tsalo · 2020-10-21T21:28:06Z

References issue #324 and PR #327. This should probably be merged before #327, so that the linting CI step passes in that PR.

Proposed Changes

Run black on the package, skipping heuristics, tests, and anything from the repo's top level.

I skipped heuristics, tests, and anything from the repo's top level.

tsalo · 2020-10-21T21:44:43Z

The E203 error in the linting step can be ignore- E203 is not PEP8 compatible and flake8 is in the wrong on this.

smoia

I went through half of the files and I saw many cases that need fixing - I know, you whised it was as simple as just running black.

Can I ask you to look at the changes? I will continue later on.

phys2bids/_version.py

phys2bids/bids.py

smoia · 2020-10-21T22:00:24Z

phys2bids/cli/run.py

@@ -22,136 +22,186 @@ def _get_parser():
    """
    parser = argparse.ArgumentParser()


Some of the parser helps got weird. we should probably manually edit them after black - unless I missed the part in which black doesn't substitute ' with " if " is contained in the string, but then it becomes incredibly ugly and confusing.

I've gone through and cleaned up the strings. Some switches between ' and " are necessary, although the internal double-quotes could be changed to single-quotes to eliminate them.

Sounds reasonable! In fact, I think they were double quote for a similar reason (not to confuse them with the external single quotes).

Awesome. I've switched them.

phys2bids/cli/run.py

smoia · 2020-10-21T22:00:57Z

phys2bids/cli/run.py

+        type=str,
+        help="Folder where output should be placed. "
+        "Default is current folder. "
+        'If "-heur" is used, it\'ll become '


phys2bids/interfaces/txt.py

smoia · 2020-10-21T22:07:38Z

phys2bids/interfaces/txt.py

+        orig_units.append(item.split(" ")[1])
+    units = [
+        "s",
+    ]


I know why black is formatting this list in this way throughout the whole file, but it's awful. can we add a #fmt: off (or whatever it was) here?

This is fixed by just not including the trailing commas in the lists (not sure why they're there). black won't reformat units = ["s"], so I'll just revert those.

After digging a bit, it turns out that trailing commas are there to make lists easily extensible, so it actually makes sense to expand any list with a comma at the end to multiple lines because you expect that list to grow and you want minimal diffs when that happens. In any case, these lists aren't likely to grow, so I just removed the trailing commas and put them on single lines.

phys2bids/interfaces/txt.py

phys2bids/phys2bids.py

smoia · 2020-10-21T22:14:54Z

phys2bids/phys2bids.py

-        LGR.warning('While "-heur" was specified, option "-sub" was not.\n'
-                    'Skipping BIDS formatting.')
+        LGR.warning(
+            'While "-heur" was specified, option "-sub" was not.\n'


Do you mean the new line is odd, or the fact that this line uses single quotes and the next uses double quotes?

eurunuela

LGTM! Thank you @tsalo !

It will take me some time to get used to the double quotes 😅

eurunuela · 2020-10-23T14:55:42Z

phys2bids/bids.py

+    # kelvin: thermodynamic temperature
+    "kelvin": "K", "kelvins": "K",
+    # mole: amount of substance
+    "mol": "mol", "mole": "mol",
+    # newton: force, weight
+    "newton": "N", "newtons": "N",
+    # pascal: pressure, stress
+    "pascal": "Pa", "pascals": "Pa", "pa": "Pa",
+    # volt: voltage (electrical potential), emf
+    "volt": "V", "volts": "V",
+    # degree Celsius: temperature relative to 273.15 K
+    "°c": "°C", "°celsius": "°C", "celsius": "°C",
+    # ampere: electric current
+    "ampere": "A", "amp": "A", "amps": "A",
+    # second: time and hertzs: frequency
+    # siemens: electric conductance (e.g. EDA)
+    "siemens": "S",
+    # second: time and hertzs
+    "1/hz": "s", "1/hertz": "s", "hz": "Hz",
+    "1/s": "Hz", "1/second": "Hz", "1/seconds": "Hz",
+    "1/sec": "Hz", "1/secs": "Hz", "hertz": "Hz",
+    "second": "s", "seconds": "s", "sec": "s",
+    "secs": "s",
+    # All the aliases with one letter (to avoid issues)
+    "k": "K", "n": "N", "v": "V", "c": "°C", "a": "A", "s": "s",


eurunuela · 2020-10-23T14:58:44Z

phys2bids/bids.py

+                        f"The given unit prefix {unit} does not "
+                        "have aliases, passing it as is"
+                    )
+                    prefix = orig_unit[: len(unit)]


Well this is new to me.

I was referring to the space after the colon.

Yes, it takes some getting used to. The PEP8 rules for colons are annoyingly complicated, but this more in line with them than what flake8 enforces.

eurunuela · 2020-11-05T16:10:00Z

phys2bids/phys2bids.py

-                    'call phys2bids using both "-ntp" and "-tr" arguments')
+        LGR.warning(
+            "Skipping trigger pulse count. If you want to run it, "
+            'call phys2bids using both "-ntp" and "-tr" arguments'


Black failed here!

Run black on files in package.

ba2a044

I skipped heuristics, tests, and anything from the repo's top level.

tsalo added the Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog label Oct 21, 2020

tsalo requested a review from smoia October 21, 2020 21:28

smoia requested changes Oct 21, 2020

View reviewed changes

smoia self-assigned this Oct 21, 2020

smoia requested a review from eurunuela October 21, 2020 22:16

tsalo added 6 commits October 21, 2020 18:49

Address review.

a09d69b

Revert _version.

c1bdf40

Ignore the dictionaries.

d4c8dff

Fix more multiline lists.

137a723

Fix multline list.

91219e8

Update phys2bids.py

b0fcdc4

tsalo requested a review from smoia October 21, 2020 23:21

Swap single and double quotes.

625af49

smoia mentioned this pull request Oct 21, 2020

Adopt Black and isort #327

Merged

eurunuela approved these changes Oct 23, 2020

View reviewed changes

smoia added Refactoring Improve nonfunctional attributes and removed Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog labels Nov 2, 2020

eurunuela reviewed Nov 5, 2020

View reviewed changes

tsalo closed this Nov 19, 2020

		@@ -22,136 +22,186 @@ def _get_parser():
		"""
		parser = argparse.ArgumentParser()

Conversation

tsalo commented Oct 21, 2020

Proposed Changes

Uh oh!

tsalo commented Oct 21, 2020

Uh oh!

smoia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eurunuela left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants