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

Fix Issue #90: Remaining length issues #91

Merged
merged 4 commits into from
Dec 22, 2021

Conversation

Eason010212
Copy link
Contributor

#90 (comment)
I override the default len() method. Now, when calculating the length of a string, it will return the length of the UTF-8 encoded bytes instead of the number of characters.
This change will not affect the calculation of English string length. However, when dealing with certain languages, it helps a lot.
For example, "中文" in UTF-8 encoded bytes is "\xe4\xb8\xad\xe6\x96\x87". Previously, len("中文") is 2 (incorrect). After override, len("中文") is 6 (len(b'\xe4\xb8\xad\xe6\x96\x87'), correct).
Because there are too many uses of len(str) in the code, I carried out this override to fix them at a time. After confirming that the issue I raised is valid, maybe you can have a more professional and elegant way to fix it.
Thank you very much.

@Eason010212 Eason010212 changed the title Fix Issue #90: Update adafruit_minimqtt.py Fix Issue #90: Remaining length issues Aug 27, 2021
@tannewt tannewt requested a review from brentru August 27, 2021 16:08
len_overrided = len
def len(object):
if isinstance(object, str):
return len_overrided(object.encode('utf-8'))
Copy link
Member

Choose a reason for hiding this comment

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

Where is the len_overrided method declared?

@@ -64,6 +64,13 @@
_default_sock = None # pylint: disable=invalid-name
_fake_context = None # pylint: disable=invalid-name

# Override default len() method
len_overrided = len
def len(object):
Copy link
Member

Choose a reason for hiding this comment

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

Is there a more elegant way we can detect this instead of overriding the len method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please review the latest commit (a2032c0) and compare it with the original version.
I removed the overloaded method and replaced it with a more polite fix (I checked the usage of all len (str) in the code and replaced them with len (bytes) if necessary to correct the calculation result of the remaining length).
Please check whether my modification is valid and correct. Thank you.

@Eason010212 Eason010212 requested a review from brentru August 28, 2021 00:31
Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested this version of library successfully with pyportal and adafruitio simpletest.

Thank you for this fix @Eason010212

@Eason010212
Copy link
Contributor Author

Eason010212 commented Dec 22, 2021 via email

@FoamyGuy FoamyGuy merged commit 3cd9d3d into adafruit:main Dec 22, 2021
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Dec 22, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_FRAM to 1.3.12 from 1.3.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_FRAM#32 from tekktrik/fix/update-value-error

Updating https://github.com/adafruit/Adafruit_CircuitPython_VL53L0X to 3.5.0 from 3.4.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_VL53L0X#21 from Smankusors/master
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 2.5.12 from 2.5.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#90 from kattni/specify-python
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 5.1.5 from 5.1.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#91 from Eason010212/fix-remaining-length
  > update rtd py version
rtwfroody pushed a commit to rtwfroody/Adafruit_CircuitPython_MiniMQTT that referenced this pull request Sep 18, 2022
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.

3 participants