WhyNotHugo / python-barcode

㊙️ Create standard barcodes with Python. No external dependencies. 100% Organic Python.
http://python-barcode.rtfd.io/
MIT License
558 stars 123 forks source link

Type hints and cleanup of Barcode.build() and surrounding code #230

Closed maresb closed 1 month ago

maresb commented 4 months ago

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.

maresb commented 4 months ago

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.

maresb commented 2 months ago

Sorry for the slow reply, these were a lot of changes and needed serious attention.

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...

maresb commented 2 months ago

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.
maresb commented 2 months ago

Ok, cool, I introduced this exception in 5ff0cd7 :joy:

maresb commented 2 months ago

@WhyNotHugo, could you please re-review? I think I've addressed your concerns. Also, I tried to make the individual commits very simple.

~One potentially confusing commit is "Parametrize test_generating_barcodes". I preserved the HTML by creating a module-scoped fixture that collects the individual HTML image elements. This way the test cases can pass or fail independently.~ (Split out into #233)

The other commit that requires some thought is "Cleanup Barcode.build()". The logic here is that only one invocation of self._convert can possibly encode a digit from charset C which results in returning None. Thus I split it out this case into a separate function _convert_or_buffer. While it's more lines-of-code, I think it's easier to understand being more type-friendly and less like a mysterious finite-state automaton.

maresb commented 2 months ago

If it's too much then let me know and I'll split it into multiple PRs.

WhyNotHugo commented 2 months ago

I've triggered a rebase after merging https://github.com/WhyNotHugo/python-barcode/pull/233.

Feel free to rebase locally and force-push if you had any pending changes.

maresb commented 2 months ago

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.

maresb commented 1 month ago

@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.

WhyNotHugo commented 1 month ago

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 commented 1 month ago

Let's squash. Thanks @WhyNotHugo!!!

WhyNotHugo commented 1 month ago

Thanks!