Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup of barcode.codex._convert #234

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Jul 9, 2024

Most invocations of _convert are guaranteed to not handle digits in charset C. Handling extra digits in charset C introduces extra complexity due to grouping of pairs of digits, which is handled by _buffer. (I rename _buffer to _digit_buffer for clarity.)

In order to isolate the complexity and ease type hinting, I restrict _convert to return only int and raise an informative exception whenever the buffer is triggered. Correspondingly I introduce a function _convert_or_buffer returning int | None to handle the full case when _digit_buffer might be used.

There is just one single place (_build) where _convert_or_buffer is necessary rather than _convert.

Reasons why _convert is safe to use in all other invocations:

  • In _new_charset it's used to switch charset, so it won't be used on a digit.
  • In _maybe_switch_charset and _build it's used to flush the buffer immediately after switching to either charset A or B, so the charset cannot be C.
  • In _convert_or_buffer I explicitly check before invoking that the charset isn't C.

This makes it clear why only the invocation in _build needs to handle the None return type. (IMO it's pretty tricky to deduce this if you're not already familiar with the implementation.)

@maresb
Copy link
Contributor Author

maresb commented Jul 9, 2024

I'd prefer merging this after #230, but either way works.

Rebasing after #230 will likely require resolving a trivial merge conflict.

@maresb
Copy link
Contributor Author

maresb commented Jul 31, 2024

@WhyNotHugo, I rebased to resolve the merge conflicts. Zero urgency on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant