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

Lint #2

Open
wants to merge 9 commits into
base: master
from
Open

Lint #2

wants to merge 9 commits into from

Conversation

@dherrada
Copy link

dherrada commented Jan 14, 2020

No description provided.

tannewt and others added 2 commits Jan 8, 2020
@dherrada dherrada requested review from tannewt and siddacious Jan 14, 2020
@@ -50,7 +52,7 @@ class _RemoteCommand(ComplexCharacteristic):
uuid = VendorUUID("9B3C81D8-57B1-4A8A-B8DF-0E56F7CA51C2")

def __init__(self):
super().__init__(properties=Characteristic.WRITE_NO_RESPONSE | Characteristic.NOTIFY,
super().__init__(properties=Characteristic.WRITE_NO_RESPONSE if Characteristic.WRITE_NO_RESPONSE else Characteristic.NOTIFY,

This comment has been minimized.

Copy link
@dherrada

dherrada Jan 14, 2020

Author

So I'm extra not sure about this one. I wasn't entirely sure what that bitwise or was doing, but I thought it was trying to conditionally assign either WRITE_NO_RESPONSE or NOTIFY based on which one was True. Sphinx really didn't like that so I changed it

This comment has been minimized.

Copy link
@dhalbert

dhalbert Jan 14, 2020

The properties are a bitmask, so it's doing a bitwise OR to set both bits.
Characteristic.WRITE_NO_RESPONSE if Characteristic.WRITE_NO_RESPONSE else Characteristic.NOTIFY is a if a else b, which is not what is intended. The values are all non-zero.

This comment has been minimized.

Copy link
@dherrada

dherrada Jan 14, 2020

Author

@dhalbert Ok. Why I was changing them was that when I tried to build the docs, it gave me this error on all of the three that did a bitwise or
TypeError: unsupported operand type(s) for |: 'WRITE_NO_RESPONSE' and 'NOTIFY'

This comment has been minimized.

Copy link
@dherrada

dherrada Jan 14, 2020

Author

Thanks for your review by the way. That was actually very helpful as I wasn't really sure what that line was doing

This comment has been minimized.

Copy link
@dhalbert

dhalbert Jan 14, 2020

They are just integers, taken from _bleio, so I think this is due to not mocking _bleio properly or something.

https://github.com/adafruit/Adafruit_CircuitPython_BLE/blob/3d7bb01776aaa06cb6455bfcb437d09d456281d8/adafruit_ble/characteristics/__init__.py#L84-L89

This comment has been minimized.

Copy link
@dherrada

dherrada Jan 14, 2020

Author

@dhalbert Ok. So I should just copy that file over to this repo and enable it, right?

This comment has been minimized.

Copy link
@dherrada

dherrada Jan 14, 2020

Author

Sorry for not responding quicker. I'm bad at checking notifications when I'm really focused on something

This comment has been minimized.

Copy link
@dhalbert

dhalbert Jan 14, 2020

No problem, I'm not sitting around waiting 😃. Right, I think you just need sys.path.insert, and the file in mocks, but do you also need adafruit-circuitpython-ble in requirements.txt? I haven't spent a lot of time on kind of thing. Maybe that's just for PyPi, and we don't need it now because we don't support BLE in blinka, but it should be there for later, I think.

Pinging @kattni and/or @siddacious for requirements.txt q ^^

This comment has been minimized.

Copy link
@siddacious

siddacious Jan 14, 2020

@dhalbert @dherrada yes, the requirements.txt wants adafruit-circuitpython-ble, I believe as Dan suggests to future proof for Blinka

This comment has been minimized.

Copy link
@dherrada

dherrada Jan 14, 2020

Author

I added it since it is imported in the module

Edit: Bryan's comment hadn't loaded in when I initially made this comment.

adafruit_ble_apple_media.py Outdated Show resolved Hide resolved
adafruit_ble_apple_media.py Outdated Show resolved Hide resolved
…y format"

This reverts commit 5245afa.
@dherrada dherrada requested a review from dhalbert Jan 14, 2020
@dherrada

This comment has been minimized.

Copy link
Author

dherrada commented Jan 14, 2020

@dhalbert So I've gotten past the tough part, getting the docs to build without any errors for adafruit_circuitpython_ble_apple_media.py. Do you mind taking a look at what I did for the BLE mock to see if I did it the right way?

@dhalbert

This comment has been minimized.

Copy link

dhalbert commented Jan 14, 2020

looks ok!

@dherrada

This comment has been minimized.

Copy link
Author

dherrada commented Jan 14, 2020

At this point, the module is passing, and all that needs to happen is some todos need to be removed, and the docs directory needs some rst files.

@dherrada dherrada requested a review from ladyada Jan 14, 2020
@dherrada

This comment has been minimized.

Copy link
Author

dherrada commented Jan 14, 2020

Yay! It's passing!

Copy link
Contributor

tannewt left a comment

Thanks for your quick on this. One request.

@@ -20,7 +22,7 @@
# Uncomment the below if you use native CircuitPython modules such as
# digitalio, micropython and busio. List the modules you use. Without it, the
# autodoc module docs will fail to generate with a warning.
# autodoc_mock_imports = ["digitalio", "busio"]
autodoc_mock_imports = ["_bleio", "adafruit_ble.services"]

This comment has been minimized.

Copy link
@tannewt

tannewt Jan 14, 2020

Contributor

Please remove these mocks and the sys.path above. The real packages in requirements.txt should be enough.

The goal is to get this OK without mocks so that we don't need mocks copied into each BLE library separately.

This comment has been minimized.

Copy link
@dhalbert

dhalbert Jan 14, 2020

If the _bleio mock in adafruit_ble is enough, yes, that's good.

This comment has been minimized.

Copy link
@dherrada

dherrada Jan 14, 2020

Author

@tannewt Ah, that makes a lot of sense. I totally forgot that actions will install the dependencies before it builds the docs

@dherrada

This comment has been minimized.

Copy link
Author

dherrada commented Jan 14, 2020

@tannewt Please correct me if I'm wrong, because there's probably something that I've missed, but without using something like importlib or a proxy importer, or maybe even a symlink, I don't believe there is a way to get Actions to use adafruit_ble from PyPI. The import statements in the module in this repo all say adafruit_ble. On PyPI, adafruit_ble is called adafruit-circuitpython-ble. This module is only designed to be used on microcontrollers running CircuitPython so that isn't an issue because if you were using this module on a microcontroller, you'd download the library from GitHub, where it is called adafruit_ble. When Actions installs BLE, it installs the one with hyphens in the name from PyPi. Because python will not import anything with a hyphen in it's name, Actions can't use the BLE from requirements.txt to build the docs. Again, I'm not super familiar with all of this stuff, so there's probably something I'm not thinking about.

Edit: I realized that the above comment might come off as a bit rude. It wasn't intended that way at all, so I apologize if that's the way it reads

@dherrada dherrada requested a review from tannewt Jan 14, 2020
@tannewt

This comment has been minimized.

Copy link
Contributor

tannewt commented Jan 14, 2020

The pypi package name is not the name that is imported. They are two separate things. The requirements.txt is telling actions to install the pypi package. After doing so, adafruit_ble should be available from python.

@dherrada

This comment has been minimized.

Copy link
Author

dherrada commented Jan 14, 2020

@tannewt So I agree that adafruit_ble should be available when you do that, however, it doesn't show up in actions, and when I try importing it locally in the repl, it doesn't show up either, although it is entirely possible that's due to my computer, and not the library itself

@tannewt

This comment has been minimized.

Copy link
Contributor

tannewt commented Jan 16, 2020

Ah, I see that too. BLE must be packaged wrong then.

Looks like this needs to be changed to package from py_module: https://github.com/adafruit/Adafruit_CircuitPython_BLE/blob/master/setup.py#L62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.