search

SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
721 stars 168 forks source link

Removing parentheses from assert statements #462

Closed thedon702 closed 1 year ago

thedon702 commented 1 year ago

Removing the parentheses from assert statements according to issue #459

fixes #459

kdmukai commented 1 year ago

fyi, @conraddonovan16 github understands certain verbs related to issues: closes #459.

When it sees that in the PR and the PR is merged, the Issue will also be auto-closed. see: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests

kdmukai commented 1 year ago

ACK tested.

pytest had no complaints!

jdlcdl commented 1 year ago

At first glance, I'm not sure why pyzbar is new here. I thought it was indeed added to our environment, and that it will appear in our local repo depending how we install it, but not sure why it's in this pr, I suspect it was added by mistake.

Not saying it shouldn't be here, just saying Im not sure I understand what's happening. I'm pretty sure ./src/pyzbar should not be part of this pr.

jdlcdl commented 1 year ago

as of 3011211

ACK tested

kdmukai commented 1 year ago

At first glance, I'm not sure why pyzbar is new here. I thought it was indeed added to our environment, and that it will appear in our local repo depending how we install it, but not sure why it's in this pr, I suspect it was added by mistake.

~Not saying it shouldn't be here, just saying Im not sure I understand what's happening.~ I'm pretty sure ./src/pyzbar should not be part of this pr.

Wow, that was a great catch @jdlcdl! When I review changes I don't pay much attention to the left-hand tree structure view. Totally missed that submodule inclusion.

jdlcdl commented 1 year ago

Actually, I breezed right by it. Halfway down I was thinking:

"This is such a nice and tight pr, one line deleted, one line added, I just have to pay attention and it's going to be acked as soon as I reach the bottom."

When I scrolled up, I noticed 41 added, 40 deleted, so I ran thru it again thinking I missed a newline. I was about to give up, then I saw it at the top.

After speaking with Conrad, we both realized that git diff doesn't make new empty directories obvious, even though it respects our wishes with git commit -am "commit message". It was harmless anyways, and it reinforces my belief not to use the -am switch, which is otherwise very convenient.

newtonick commented 1 year ago

ACK and Tested