Skip to content

Fix ValueError thrown on specific byte sequence#4

Closed
adamsqi wants to merge 3 commits intodlenski:masterfrom
delimitry:master
Closed

Fix ValueError thrown on specific byte sequence#4
adamsqi wants to merge 3 commits intodlenski:masterfrom
delimitry:master

Conversation

@adamsqi
Copy link
Copy Markdown

@adamsqi adamsqi commented Dec 8, 2022

There is this issue that appears in the recent version of the package:

ValueError: b'\r\n' is not in list

The bug has been fixed in the recent version of https://github.com/delimitry/aztec_code_generator
More about the issue and the fix: delimitry#7

@dlenski
Copy link
Copy Markdown
Owner

dlenski commented Dec 8, 2022

This PR doesn't apply to the master branch of this repository at all. Did you try it?

@adamsqi
Copy link
Copy Markdown
Author

adamsqi commented Dec 8, 2022

@dlenski
It doesn't apply, my bad. I forked the repo and can see lots of conflicts.

@dlenski
Copy link
Copy Markdown
Owner

dlenski commented Dec 8, 2022

As far as I can tell, the only part of the changes here that are actually relevant to the stated purpose are the following lines:

diff --git a/aztec_code_generator.py b/aztec_code_generator.py
index 65bbb28..844b8b5 100644
--- a/aztec_code_generator.py
+++ b/aztec_code_generator.py
@@ -351,7 +353,8 @@ def find_optimal_sequence(data):
                         last_mode = abbr_modes.get(char.replace('/S', '').replace('/L', ''))
                         break
                 if last_mode == 'punct':
-                    if cur_seq[x][-1] + c in punct_2_chars:
+                    # do not use mixed mode for '\r\n' as in mixed mode '\r' and '\n' are separate
+                    if cur_seq[x][-1] + c in punct_2_chars and x != 'mixed':
                         if cur_len[x] < next_len[x]:
                             next_len[x] = cur_len[x]
                             next_seq[x] = cur_seq[x][:-1] + [cur_seq[x][-1] + c]

Do you agree with that?

@adamsqi
Copy link
Copy Markdown
Author

adamsqi commented Dec 8, 2022

@dlenski
Yes it looks like this is the significant part except of formatting.

dlenski added a commit that referenced this pull request Dec 8, 2022
There is a bug where we attempt to encode '\r\n' in an impossible way in
MIXED mode; see delimitry#7

I'm not convinced that the solution from
#4 is a very clarifying
or complete one.

For now, just add a test which demonstrates the problem, and is marked as
expectedFailure.
dlenski added a commit that referenced this pull request Dec 8, 2022
There is a bug where we attempt to encode '\r\n' in an impossible way in
MIXED mode; see delimitry#7

I'm not convinced that the solution from
#4 is a very clarifying
or complete one.

For now, just add a test which demonstrates the problem, and is marked as
expectedFailure.
@dlenski
Copy link
Copy Markdown
Owner

dlenski commented Dec 8, 2022

I'm not convinced that the solution from #4 is a very clarifying or complete one. For now, I've just added a (failing) test to demonstrate the problem in a94f699.

dlenski added a commit that referenced this pull request Dec 8, 2022
There is a bug where we attempt to encode '\r\n' in an impossible way in
MIXED mode; see delimitry#7

I'm not convinced that the solution from
#4 is a very clarifying
or complete one.

For now, just add a test which demonstrates the problem, and is marked as
expectedFailure.
dlenski added a commit that referenced this pull request Dec 8, 2022
There is a bug where we attempt to encode '\r\n' in an impossible way in
MIXED mode; see delimitry#7

I'm not convinced that the solution from
#4 is a very clarifying
or complete one.

For now, just add a test which demonstrates the problem, and is marked as
expectedFailure.
@adamsqi
Copy link
Copy Markdown
Author

adamsqi commented Dec 9, 2022

@dlenski
I think this PR can be closed, since the fix needs to be added in a new PR.

@adamsqi adamsqi closed this Dec 12, 2022
dlenski added a commit that referenced this pull request Dec 17, 2024
See #4.

I thought there'd be a more elegant fix here, but in the end I went with the
same one as inhttps://github.com/delimitry/pull/8/files#diff-bf7c11e2b0865687a2785c4fa2a55920d92137aa2b6548c5f87835d98f73339fR356-R357
dlenski added a commit that referenced this pull request Dec 17, 2024
See #4.

I thought there'd be a more elegant fix here, but in the end I went with the
same one as inhttps://github.com/delimitry/pull/8/files#diff-bf7c11e2b0865687a2785c4fa2a55920d92137aa2b6548c5f87835d98f73339fR356-R357
dlenski added a commit that referenced this pull request Dec 17, 2024
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.

ValueError is thrown on specific byte sequence

3 participants