Type hints and cleanup of Barcode.build() and surrounding code #230
Conversation
|
There's so much to do here. I'm grinding away, fixing stuff that catches my eye. If you let me know what you like and don't like, I can eventually try and rebase out the good parts. |
There was a problem hiding this comment.
Sorry for the slow reply, these were a lot of changes and needed serious attention.
The grand majority of this is fine; I only see one problem (which breaks existing usages and tests).
This also makes mypy fail for barcode/writer.py, but I can address that myself separately (you're only surfacing an existing issus).
barcode/base.py
Outdated
|
|
||
| def __repr__(self) -> str: | ||
| return f"<{self.__class__.__name__}({self.get_fullcode()!r})>" | ||
|
|
||
| def build(self) -> list[str]: | ||
| """Return a singleton list with a string encoding the barcode as 1s and 0s.""" |
There was a problem hiding this comment.
nitpick: this isn't a singleton (a class which allows only one instance), it's a list with a single item (completely different things).
I think that some barcodes have other values (like G) to encode guard lines.
There was a problem hiding this comment.
Ah, I'm using math terminology here. Thanks for pointing out the conflicting meaning, I'll change it!
I think that some barcodes have other values (like G) to encode guard lines.
Oh, ya, I see that now in EAN13, thanks!
There was a problem hiding this comment.
Oh, I didn't know about the mathematics concept. I've learnt something today :)
barcode/__init__.py
Outdated
| return get_barcode(name) | ||
|
|
||
|
|
||
| def generate( | ||
| name: str, | ||
| code: str, | ||
| writer: BaseWriter | None = None, | ||
| output: str | (os.PathLike | (BinaryIO | None)) = None, | ||
| writer: BaseWriter | None, |
There was a problem hiding this comment.
This parameter was previously optional. With this change, it is no longer optional.
Some tests fail because of this. You can run them with pytest.
There was a problem hiding this comment.
I'm changing this back.
I'm not defending it, but for disclosure I think for some reason I wanted output to never not be None. This forced me to remove the default value. Since positional args can't come after kwargs, this forced me to remove the default value on writer.
barcode/writer.py
Outdated
| @@ -204,65 +219,60 @@ def packed(self, line: str) -> Generator[tuple[int, float], str, None]: | |||
| yield (-c, self.guard_height_factor) | |||
| c = 1 | |||
|
|
|||
| def render(self, code): | |||
| def render(self, code: list[str]) -> Any: | |||
There was a problem hiding this comment.
Don't use Any; just leave the type hint absent until we can address it.
Ya, honestly I felt bad for dumping this mess on you. Thanks so much for sifting through it!!! This looks quick, let me see if I can crank this out now... |
|
I got pytest running. Now I'm looking into: > raise RuntimeError(
f"Character {char} could not be converted in charset {self._charset}."
)
E RuntimeError: Character 9 could not be converted in charset C. |
|
Ok, cool, I introduced this exception in 5ff0cd7 😂 |
|
@WhyNotHugo, could you please re-review? I think I've addressed your concerns. Also, I tried to make the individual commits very simple.
The other commit that requires some thought is "Cleanup Barcode.build()". The logic here is that only one invocation of |
|
If it's too much then let me know and I'll split it into multiple PRs. |
maresb
force-pushed
the
cleanup-build
branch
2 times, most recently
from
0f9c80f
to
0c671cd
Compare
I'm not 100% sure this is correct, but I think a None value would lead to an error later on anyways.
Also add some type hints.
It's part of the standard library, and we're not aiming for performance
|
I've triggered a rebase after merging #233. Feel free to rebase locally and force-push if you had any pending changes. |
|
Thanks @WhyNotHugo!!! I agree with the rebase. I split off the non-minimal changes into #234, so the only unreviewed commits are the last four, directly addressing your feedback above, and I'd expect those four to be super-quick to review. |
|
@WhyNotHugo, there is no urgency whatsoever on this, but it seems like you reviewed all but the last four small commits, so perhaps this is simple to merge. |
|
I think this is okay to merge. There are commits that make changes and later commits that revert them. Do you want to rebase this? Otherwise I'll just squash them into a single commit. |
maresb
changed the title
|
Let's squash. Thanks @WhyNotHugo!!! |
|
Thanks! |
This is a follow-up to #228. I'm trying to not break backwards-compatibility for now, but just to make the code more understandable.