Skip to content

Fixed minor formatting in docs #39

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

Closed
wants to merge 3 commits into from

Conversation

AlexColello
Copy link
Contributor

I wanted to fix a minor issue in the docs where a code block in a comment did not completely enclose the sample code. Along the way I also ended up dealing with an issue where PyLint found too many duplicate lines of code between clue_ams_remote.py and clue_ams_remote_advanced.py.

This will fix a formatting issue in the documentation of the clue object where the sample code is not in the code block.
@ladyada ladyada requested a review from kattni February 22, 2021 15:52
Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

Thanks for catching the docs issue! And thank you for noticing that Pylint was not running on the subfolders. I believe you may have caught something we need to fix on a few other repos as well.

Please see the suggested change below for how to update the build.yml file.

Comment on lines 56 to 57
([[ ! -d "examples" ]] || pylint --disable=missing-docstring,invalid-name,bad-whitespace $( find . -name "advanced_examples" -prune -false -o -path "./examples/*.py" ))
([[ ! -d "examples/advanced_examples" ]] || pylint --disable=missing-docstring,invalid-name,bad-whitespace $( find . -path "./examples/advanced_examples/*.py" ))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try the following instead of creating a new line to check the examples subfolder. It worked for me locally, but I would appreciate it if you could test it as well.

Suggested change
([[ ! -d "examples" ]] || pylint --disable=missing-docstring,invalid-name,bad-whitespace $( find . -name "advanced_examples" -prune -false -o -path "./examples/*.py" ))
([[ ! -d "examples/advanced_examples" ]] || pylint --disable=missing-docstring,invalid-name,bad-whitespace $( find . -path "./examples/advanced_examples/*.py" ))
([[ ! -d "examples" ]] || pylint --disable=missing-docstring,invalid-name,bad-whitespace $( find examples -name \*.py ))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested that change in AlexColello@3f957cb, but it causes the same error as in 91674a1. I checked the find command and both this and the original produce the same results, so I don't think that is the issue.

@AlexColello
Copy link
Contributor Author

It looks like there is some change in behavior with PyLint version 2.7.0 that is causing the error. I am not sure which behavior is absolutely correct, but for the scope of this request, keeping it at the older version seems best.

@kattni
Copy link
Contributor

kattni commented Feb 23, 2021

@AlexColello Good catch, you are correct. This issue was reported by someone else as well. However, I'm pretty sure you're also correct that it's not linting the subfolders - I tested it on another repo with subfolders in examples, and it didn't seem to catch an issue present in a subfoldered example. Please sit tight and I'm going to look into the Pylint update issue tomorrow. Thank you for your patience.

@AlexColello
Copy link
Contributor Author

@kattni I see, that probably means that 2.7 fixed an issue that was hiding the linting errors. Here is the new error for reference

************* Module clue_ble_color_patchwork
examples/clue_ble_color_patchwork.py:1:0: R0801: Similar lines in 2 files
==clue_ams_remote:20
==clue_ams_remote_advanced:28
radio = adafruit_ble.BLERadio()  # pylint: disable=no-member
a = SolicitServicesAdvertisement()
a.solicited_services.append(AppleMediaService)
radio.start_advertising(a)

while not radio.connected:
    pass

print("connected")

known_notifications = set()

i = 0
if radio.connected:
    for connection in radio.connections:
        if not connection.paired:
            connection.pair()
            print("paired")

        ams = connection[AppleMediaService]
 (duplicate-code)
examples/clue_ble_color_patchwork.py:1:0: R0801: Similar lines in 2 files
==clue_ams_remote:49
==clue_ams_remote_advanced:120
        if clue.touch_0:
            ams.previous_track()
            time.sleep(0.25)

        # Capacitive touch pad marked 1 toggles pause/play
        if clue.touch_1:
            ams.toggle_play_pause()
            time.sleep(0.25)

        # Capacitive touch pad marked 2 advances to the next track
        if clue.touch_2:
            ams.next_track()
            time.sleep(0.25)

        # If button B (on the right) is pressed, it increases the volume
        if "B" in clue.were_pressed:
            ams.volume_up() (duplicate-code)
examples/clue_ble_color_patchwork.py:1:0: R0801: Similar lines in 2 files
==clue_ams_remote:67
==clue_ams_remote_advanced:139
        while "B" in clue.were_pressed:
            ams.volume_up()
            time.sleep(0.07)

    # If button A (on the left) is pressed, the volume decreases
    if "A" in clue.were_pressed:
        ams.volume_down() (duplicate-code)

@kattni
Copy link
Contributor

kattni commented Feb 23, 2021

That is correct, @AlexColello. I am currently working on our solution. I'll get back to you soon.

@AlexColello AlexColello closed this Mar 9, 2021
@kattni
Copy link
Contributor

kattni commented Mar 9, 2021

@AlexColello My apologies for letting this slide. We sorted out a solution for the issue. If you want to submit a new PR with the documentation change, it will run with the new Pylint setup and should pass. Again, I'm sorry for neglecting this PR.

@AlexColello
Copy link
Contributor Author

@kattni I submitted a new PR at #40

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.

2 participants