Codecov Report

Merging #947 (0ca23b1) into main (4d33cc6) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main     #947      +/-   ##
==========================================
- Coverage   68.75%   68.75%   -0.01%     
==========================================
  Files          63       63              
  Lines        9437     9440       +3     
==========================================
+ Hits         6488     6490       +2     
- Misses       2949     2950       +1     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d33cc6...0ca23b1. Read the comment docs.

New to this - should I reformat formatter.py with black and resubmit the code?

Looks like you got it, thank you! Sorry for the delay here.

A note on line 128. token = token.parent causes some problems with mypy because token is type _TokenType while token.parent is Optional[_TokenType]

I like the second one better, that seems truer to the code as it's currently written. Feel free to change the code though if it's easier to type, e.g. using a new temp variable or adding a new "if t is None" bailout.

Thanks, @thomasballinger. I think the only way to avoid using Any as a type is to use a temporary variable - the reason being that token has to be _TokenType (because of the line token = token.parent ... None cannot equal None.parent) and token.parent has to be Optional[_TokenType] because that method can return either a token or None.

At least, that's how it seems to me. I'll try another commit with a temp variable and you can make the call. I'm probably using too much of your time on the little details, but I appreciate the help!

@thomasballinger, is there anything else I need to do with this PR at this point?

Sorry about the delay, pinging with that comment was the perfect action. I'll look into this more this morning, right now when I run mypy locally I'm getting

(bpython) tomb ((0ca23b1...)) bpython$ mypy
bpython/formatter.py:114: error: No overload variant of "__init__" of "Formatter" matches argument type "Dict[str, str]"
bpython/formatter.py:114: note: Possible overload variants:
bpython/formatter.py:114: note:     def __init__(self, *, encoding: None = ..., outencoding: None = ..., **options: Any) -> None
bpython/formatter.py:114: note:     def __init__(self, *, encoding: str, outencoding: None = ..., **options: Any) -> None
bpython/formatter.py:114: note:     def __init__(self, *, encoding: None = ..., outencoding: str, **options: Any) -> None

which I'm sure it just my problem since it works in CI. Once I get this sorted (no action needed from you) I'll merge this.

I have to change the typing of **options to Any (which means Dict[str, Any] when used with **), which matches Pygments typing, to make this work locally for me. But it works without this change in CI. I figured this meant that my local enviroment was different than CI and CI should win, but I get the same thing from a fresh checkout on a linux box.

diff --git a/bpython/formatter.py b/bpython/formatter.py
index 20573bd..f216f21 100644
--- a/bpython/formatter.py
+++ b/bpython/formatter.py
@@ -28,7 +28,7 @@
 # mypy: disallow_untyped_calls=True


-from typing import MutableMapping, Iterable, TextIO
+from typing import Any, MutableMapping, Iterable, TextIO
 from pygments.formatter import Formatter
 from pygments.token import (
     _TokenType,
@@ -102,7 +102,7 @@ class BPythonFormatter(Formatter):
     straightforward."""

     def __init__(
-        self, color_scheme: MutableMapping[str, str], **options: str
+        self, color_scheme: MutableMapping[str, str], **options: Any
     ) -> None:
         self.f_strings = {}
         for k, v in theme_map.items():

I could imagine a reason for this: according to the types, it's not safe to include both encoding and outencoding with string values. Since I can do this with a fresh checkout, I think there's something wrong with our CI that we should fix, but in the meantime we can just make this change. I'll add this commit and merge.

Merged, thank you @Ben-Reg!