8000 Fix emc2101_enums import by caternuson · Pull Request #32 · adafruit/Adafruit_CircuitPython_EMC2101 · GitHub
[go: up one dir, main page]
Skip to content

Fix emc2101_enums import #32

New issue

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

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 17, 2023
Merged

Fix emc2101_enums import #32

merged 3 commits into from
Sep 17, 2023

Conversation

caternuson
Copy link
Contributor

For #31.

Tested on QT PY RP2040:

Adafruit CircuitPython 8.2.2 on 2023-07-31; Adafruit QT Py RP2040 with rp2040
>>> import board
>>> from adafruit_emc2101 import EMC2101
>>> i2c = board.STEMMA_I2C()
>>> emc = EMC2101(i2c)
>>> emc.spinup_drive
3
>>> emc.spinup_drive = 0
>>> emc.spinup_drive
0
>>>

@caternuson caternuson requested a review from a team August 28, 2023 20:56
@vlycop
Copy link
vlycop commented Aug 29, 2023

Hi ! OP here
What about L413 and L478 ? Should they be fixed to ?

Also i am very surprise that any other part of this library don't need this .

Not patching the other line cause spinup_time to not work

(venv) pi@prusabox:~/test_emc2101 $ vi /home/pi/test_emc2101/venv/lib/python3.7/site-packages/adafruit_emc2101/__init__.py 
(venv) pi@prusabox:~/test_emc2101 $ rm -r /home/pi/test_emc2101/venv/lib/python3.7/site-packages/adafruit_emc2101/__pycache__
(venv) pi@prusabox:~/test_emc2101 $ python
Python 3.7.3 (default, Jun 29 2023, 18:03:57) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import board
>>> from adafruit_emc2101.emc2101_lut import EMC2101_LUT as EMC2101
>>> i2c = board.I2C()
>>> emc = EMC2101(i2c)
>>> emc.spinup_drive
1
>>> emc.spinup_drive = 3
>>> emc.spinup_drive
3
>>> emc.spinup_time
4
>>> emc.spinup_time = 7
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/pi/test_emc2101/venv/lib/python3.7/site-packages/adafruit_emc2101/__init__.py", line 413, in spinup_time
    from emc2101_enums import SpinupTime
ModuleNotFoundError: No module named 'emc2101_enums'

while doing so fix the issue

(venv) pi@prusabox:~/test_emc2101 $ vi /home/pi/test_emc2101/venv/lib/python3.7/site-packages/adafruit_emc2101/__init__.py 
(venv) pi@prusabox:~/test_emc2101 $ rm -r /home/pi/test_emc2101/venv/lib/python3.7/site-packages/adafruit_emc2101/__pycache__
(venv) pi@prusabox:~/test_emc2101 $ python
Python 3.7.3 (default, Jun 29 2023, 18:03:57) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import board
>>> from adafruit_emc2101.emc2101_lut import EMC2101_LUT as EMC2101
>>> i2c = board.I2C()
>>> emc = EMC2101(i2c)
>>> emc.spinup_drive
3
>>> emc.spinup_drive = 1
>>> emc.spinup_drive
1
>>> emc.spinup_time
7
>>> emc.spinup_time = 4
>>> emc.spinup_time
4
>>>

@caternuson
Copy link
Contributor Author

Related forum post:
https://forums.adafruit.com/viewtopic.php?t=204056

Good catch. Those other imports should also be updated. Pushed those changes.

vlycop
vlycop reviewed Aug 29, 2023
View reviewed changes
Copy link
@vlycop vlycop left a comment

Choose a reason for hiding this comment

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

I am very confused by that random . so i started looking around...

I have no idea if this is better or not, but bot work so any of those version could be merged into a pip release :)

adafruit_emc2101/__init__.py Outdated
@@ -410,7 +410,7 @@ def spinup_time(self, spin_time):
# Not importing at top level so the SpinupTime is not loaded
# unless it is required, and thus 1KB bytecode can be avoided.
# pylint: disable=import-outside-toplevel
from emc2101_enums import SpinupTime
from .emc2101_enums import SpinupTime
Copy link

Choose a reason for hiding this comment

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

Suggested change
from .emc2101_enums import SpinupTime
from adafruit_emc2101.emc2101_enums import SpinupTime

Is this the same or better ? both work, and i'm not a dev ><

8000
adafruit_emc2101/__init__.py Outdated
@@ -443,7 +443,7 @@ def spinup_drive(self, spin_drive):
# Not importing at top level so the SpinupDrive is not loaded
# unless it is required, and thus 1KB bytecode can be avoided.
# pylint: disable=import-outside-toplevel
from emc2101_enums import SpinupDrive
from .emc2101_enums import SpinupDrive
Copy link

Choose a reason for hiding this comment

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

Suggested change
from .emc2101_enums import SpinupDrive
from adafruit_emc2101.emc2101_enums import SpinupDrive

adafruit_emc2101/__init__.py Outdated
@@ -473,7 +473,7 @@ def conversion_rate(self, rate):
# Not importing at top level so the ConversionRate is not loaded
# unless it is required, and thus 1KB bytecode can be avoided.
# pylint: disable=import-outside-toplevel
from emc2101_enums import ConversionRate
from .emc2101_enums import ConversionRate
Copy link

Choose a reason for hiding this comment

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

Suggested change
from .emc2101_enums import ConversionRate
from adafruit_emc2101.emc2101_enums import ConversionRate

@caternuson
Copy link
Contributor Author

TBH - This is one of the python-isms I'm never sure about myself. I actually like your approach better though.

FoamyGuy
FoamyGuy approved these changes Sep 17, 2023
View reviewed changes
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.

This looks good to me. Thanks for the fix @caternuson and reporting the issue @vlycop

I pushed one commit to tweak to use the full name for the import. I am in agreement with both of you about preferring that one, though I also do not know if there is a technical difference.

@FoamyGuy FoamyGuy merged commit ac84a24 into adafruit:main Sep 17, 2023
@FoamyGuy FoamyGuy mentioned this pull request Sep 17, 2023
Closed
8000 @caternuson
Copy link
Contributor Author

@FoamyGuy thanks for updating and closing this one up!

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Sep 18, 2023
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@30e10ce)
364c439 
Updating https://github.com/adafruit/Adafruit_CircuitPython_EMC2101 to 1.2.3 from 1.2.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_EMC2101#32 from caternuson/iss31

Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 5.0.2 from 5.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#131 from fasteddy516/improved_reset

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlycop vlycop vlycop left review comments

@FoamyGuy FoamyGuy FoamyGuy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@caternuson @vlycop @FoamyGuy
0