Port type verification #425

m-roberts · 2021-08-10T11:00:35Z

Closes #338

codecov · 2021-08-10T11:01:08Z

Codecov Report

Merging #425 (b87d1c3) into master (854e835) will decrease coverage by 4.29%.
The diff coverage is 5.88%.

❗ Current head b87d1c3 differs from pull request most recent head 56d182b. Consider uploading reports for the commit 56d182b to get more accurate results

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
- Coverage   58.56%   54.26%   -4.30%     
==========================================
  Files          72       80       +8     
  Lines        2756     3282     +526     
==========================================
+ Hits         1614     1781     +167     
- Misses       1142     1501     +359

Flag	Coverage Δ
unittests	`54.26% <5.88%> (-4.30%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pitop/core/mixins/recreatable.py	`45.45% <ø> (ø)`
pitop/pma/button.py	`42.10% <0.00%> (-11.23%)`	⬇️
pitop/pma/buzzer.py	`42.10% <0.00%> (-11.23%)`	⬇️
pitop/pma/encoder_motor.py	`72.22% <ø> (ø)`
pitop/pma/led.py	`42.10% <0.00%> (-11.23%)`	⬇️
pitop/pma/adc_base.py	`36.58% <20.00%> (-2.31%)`	⬇️
pitop/robotics/simple_pid/PID.py
pitop/robotics/simple_pid/__init__.py
pitop/miniscreen/miniscreen.py	`48.33% <0.00%> (ø)`
pitop/miniscreen/oled/core/__init__.py	`100.00% <0.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7614ea...56d182b. Read the comment docs.

for more information, see https://pre-commit.ci

ghost · 2021-08-11T23:31:13Z

This PR address the related issue #338

When using PMA sensors, checks that the user has entered a valid port - both a sensible value like D0, and also that the port type is valid for the particular sensor being used.

m-roberts · 2021-08-11T23:42:26Z

pitop/pma/button.py

+        if port_name not in Port.keys():
+            raise ValueError(f"{port_name} is not a valid port name. An example of a valid port name is D0")
+
+        if not port_name.startswith("D"):
+            raise ValueError(f"{port_name} is not a valid port type for a button. Try using a digital port, such as D0")
+


I think that this should be handled by a mixin (see Stateful, Recreatable...) so that it doesn't need to be duplicated. Not sure what I'd call it though...

Yeah I was thinking the same, I'd just call it "AnalogComponent" or "DigitalComponent". I wouldn't necessarily call it a "mixin" though, it's just standard inheritance (Button is a DigitalComponent). Admittedly the name DigitalComponent does kind of suggest it does more than just port checking, so maybe "mixin" is the right terminology and it should be called DigitalPortValidator 😃

Actually, Button inherits from gpiozero's Button class. It uses Stateful and Recreatable mixins (which are included, not inherited) - the same would be the case for this?

Well yeah but it's still inheritance - the only difference between a mixin and inheritance is generally just if the parent can't be used standalone it's called a "mixin". But then that definition doesn't really apply to abstract base classes, which is still inherited and definitely can't be used standalone. Anyway this is just terminology at this point, I think the idea makes sense

for more information, see https://pre-commit.ci

lgtm-com · 2021-08-12T13:05:30Z

This pull request introduces 2 alerts when merging 43c9fcd into 4b4d30d - view on LGTM.com

new alerts:

2 for Unused import

…-top-Python-SDK into port-type-verification

lgtm-com · 2021-08-12T13:42:06Z

This pull request introduces 2 alerts when merging 69fffe2 into 4b4d30d - view on LGTM.com

new alerts:

2 for Unused import

for more information, see https://pre-commit.ci

lgtm-com · 2021-08-23T11:45:29Z

This pull request introduces 2 alerts when merging cdbdeb3 into 4b4d30d - view on LGTM.com

new alerts:

2 for Unused import

m-roberts · 2021-08-23T12:18:14Z

examples/pma/button.py

@@ -1,7 +1,7 @@
 from pitop import Button
 from time import sleep

-button = Button("D5")
+button = Button("D8")


I don't think this is relevant..?

m-roberts · 2021-08-23T12:19:35Z

pitop/core/mixins/digital_component_checks.py

+    def __init__(self, port_name):
+        self.port_name = port_name
+
+        if port_name not in valid_digital_ports:


I think I would prefer to see regex used here to match ports: D[0-7]

m-roberts · 2021-08-23T12:21:02Z

pitop/pma/adc_base.py

@@ -22,6 +23,12 @@ class ADCBase(Stateful, Recreatable):
    """

    def __init__(self, port_name, pin_number=1, name="adcbase"):
+        if port_name not in Port.keys():
+            raise ValueError(f"{port_name} is not a valid port name. An example of a valid port name is A0")


It's usually better to create a new Exception class specific for things like this, but we don't really have much in the way of uniform exception handling. Perhaps this should be created as a new Issue for across the SDK rather than tackling that here.

m-roberts · 2021-08-23T12:21:29Z

pitop/pma/ultrasonic_sensor.py

        self._pma_port = port_name
        self.name = name

+        if port_name not in Port.keys():
+            raise ValueError(f"{port_name} is not a valid port name. An example of a valid port name is D0 or A1")


Same thing here re: regex and exceptions.

Now uses regex instead of dictionary list, as per Mike's suggestion. Needs proper testing though.

for more information, see https://pre-commit.ci

lgtm-com · 2021-09-02T14:42:01Z

This pull request introduces 5 alerts when merging 32e3dc8 into dda2052 - view on LGTM.com

new alerts:

3 for Missing call to `__init__` during object initialization
2 for Unused import

lgtm-com · 2021-09-02T15:02:13Z

This pull request introduces 3 alerts when merging 5f1cd5b into dda2052 - view on LGTM.com

new alerts:

3 for Missing call to `__init__` during object initialization

ghost · 2021-09-02T21:28:36Z

@m-roberts I think this one's ready to be merged. I took into account your comments and used regex instead of dictionaries. Have tested it, and it's passed those automatic checks too. Do I just tag you here to request for it to be merged?

lgtm-com · 2021-09-03T14:09:13Z

This pull request introduces 3 alerts when merging 56d182b into a7614ea - view on LGTM.com

new alerts:

3 for Missing call to `__init__` during object initialization

Initial changes

b87d1c3

Wil Bennett and others added 4 commits August 11, 2021 12:23

Merge branch 'master' into port-type-verification

125c3ae

Added ultrasonic check and missing port declarations

9106a90

[pre-commit.ci] auto fixes from pre-commit.com hooks

2a60bb5

for more information, see https://pre-commit.ci

Merge branch 'master' into port-type-verification

8933110

ghost marked this pull request as ready for review August 11, 2021 23:28

m-roberts commented Aug 11, 2021

View reviewed changes

Wil Bennett and others added 2 commits August 12, 2021 13:56

Changed digital port check to mixin type

39be2b3

[pre-commit.ci] auto fixes from pre-commit.com hooks

43c9fcd

for more information, see https://pre-commit.ci

m-roberts marked this pull request as draft August 12, 2021 13:11

Wil Bennett added 2 commits August 12, 2021 14:33

Added missing DigitalComponentChecks

12d0a83

Merge branch 'port-type-verification' of https://github.com/pi-top/pi…

69fffe2

…-top-Python-SDK into port-type-verification

duwudi and others added 3 commits August 23, 2021 17:15

Remove redundant assert statement

3dcd4a3

line length fix

cc758cb

[pre-commit.ci] auto fixes from pre-commit.com hooks

cdbdeb3

for more information, see https://pre-commit.ci

m-roberts commented Aug 23, 2021

View reviewed changes

Wil Bennett and others added 3 commits September 2, 2021 17:13

Changed to use regex

3168e0b

Now uses regex instead of dictionary list, as per Mike's suggestion. Needs proper testing though.

Merge branch 'master' into port-type-verification

d1f2e03

[pre-commit.ci] auto fixes from pre-commit.com hooks

32e3dc8

for more information, see https://pre-commit.ci

Removed unused imports

5f1cd5b

Merge branch 'master' into port-type-verification

56d182b

Port type verification #425

Are you sure you want to change the base?

Port type verification #425

Uh oh!

Conversation

m-roberts commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ghost commented Aug 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-roberts Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Aug 12, 2021

Uh oh!

lgtm-com bot commented Aug 12, 2021

Uh oh!

lgtm-com bot commented Aug 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Sep 2, 2021

Uh oh!

lgtm-com bot commented Sep 2, 2021

Uh oh!

ghost commented Sep 2, 2021

Uh oh!

lgtm-com bot commented Sep 3, 2021

Uh oh!

Uh oh!

m-roberts commented Aug 10, 2021 •

edited

Loading

codecov bot commented Aug 10, 2021 •

edited

Loading

m-roberts Aug 12, 2021 •

edited

Loading