Skip to content

Conversation

@DJDevon3
Copy link
Contributor

My first EVER commit. Crossing my fingers this works.

DJDevon3 added 2 commits June 29, 2022 10:49
My first EVER commit. Crossing my fingers this works.
I get credit now as a contributor. Sweet.
@tannewt tannewt requested a review from a team June 29, 2022 18:00
@FoamyGuy
Copy link
Contributor

Hooray! Thank you for submitting this @DJDevon3

It looks like you've got the fork and commit / push / create PR process done correctly, so congrats on your first one 🎉

A few things to touch up with the code and added files themself though:

We use Black code formatting for all of the library and example code. The easiest way to do it is to run all of the same checks that we do automatically on github at once using a tool called pre-commit we have a guide page here that shows how to install and use it: https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/check-your-code

Specifically this PR did not pass the check due to the code formatting (which pre-commit will take care of if you run it) as well as missing license files for the new .bmp images. All files in the repo must have a license associated with them. For image files this is a text file with the same name and .license appended to the end with the appropriate license header strings in it. Check the license files for the existing .bmp images as an example of what goes inside.

You can run pre-commit and add those license files locally and then make a new commit and push to this branch and it should automatically show up here in the PR and trigger the checks to re-run again.

Let me know if you have any issues I or someone on our discord will be able to help you work through them.

Thanks again.

DJDevon3 and others added 4 commits July 4, 2022 14:20
Attempted to install pre-commit, unsure if it was successful.
fixed according to github pylint recommendations
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.

I made a few small tweaks:

Removed I2C bus initialization. It wasn't used by the rest of the script, and in cases where the user has no other devices connected to the bus it can raise an exception (this occurred to me during testing).

Added display.release_displays() before initializing the external display. This is typically best practice with external displays. If not done it can cause trouble if the user attempts to run the same script a second (or more) times.

Removed a comment about 2.5" featherwing device. The closest one I could find on adafruit.com was 2.4" and it uses a different driver so it would require a different import and display initialization code in order to be supported. It could be added later commented out if someone would like to.

I tested the example from this PR with these tweaks successfully on a Feather RP2040 with 3.5" TFT Featherwing.

Thanks again for submitting this @DJDevon3

@FoamyGuy FoamyGuy merged commit 2cec797 into adafruit:main Jul 11, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 12, 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.

2 participants