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

Update codex.py #56

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

Update codex.py #56

wants to merge 2 commits into from

Conversation

srolin
Copy link

@srolin srolin commented Oct 28, 2019

Fixes for bug #55
writer_options{"add_checksum": False} being ignored for Code39 barcodes.
Added class variable to track when the checksum has been added so it can't add twice
Moved the check for checksum to the render function
Set a default value class variable.

Fixes for bug WhyNotHugo#55
writer_options{"add_checksum": False} being ignored for Code39 barcodes.
Added class variable to track when the checksum has been added so it can't add twice
Moved the check for checksum to the render function
Set a default value class variable.
Copy link
Owner

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to make sense to me -- tho I can't see how the checksum got calculated twice. Have you figured out where that happened?

barcode/codex.py Outdated
@@ -61,7 +61,7 @@ def get_fullcode(self):
return self.code

def calculate_checksum(self):
check = sum(code39.MAP[x][0] for x in self.code) % 43
check = sum([code39.MAP[x][0] for x in self.code]) % 43
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List comprehension seems redundant here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree, not sure how that line got changed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I didn't see an example of it being added twice, but if anyone called the render function twice then it would.
Agreed on list comprehension

barcode/codex.py Outdated Show resolved Hide resolved
@WhyNotHugo
Copy link
Owner

render doesn't seem to modify self.code. How does the checksum end up being added twice? It would seem that it's only done in the constructor (or am I missing something?).

@srolin
Copy link
Author

srolin commented Nov 1, 2019

render doesn't seem to modify self.code. How does the checksum end up being added twice? It would seem that it's only done in the constructor (or am I missing something?).

This was changed in the first commit. I moved the checksum code from the class constructor to the render function. The second commit was just to address your previous comments. If you look at the combined changes you can see the new render code.

@WhyNotHugo
Copy link
Owner

Sorry, I'm not seeing it. 😓
Can you point me to the exact line where this happens? Or maybe a sample code to reproduce the issue? Or a unit test that fails for the bug?

@srolin
Copy link
Author

srolin commented Nov 5, 2019

I think perhaps I have not explained the change very well. I am traveling at the moment, but will take some time and fully document the bug, the fix, what code breaks etc. Thanks for your patience.

@WhyNotHugo
Copy link
Owner

Hey there! Do you have any update?

Base automatically changed from master to main March 9, 2021 18:56
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

3 participants