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

Removing parentheses from assert statements #462

Conversation

thedon702
Copy link
Contributor

@thedon702 thedon702 commented Aug 28, 2023

Removing the parentheses from assert statements according to issue #459

fixes #459

@kdmukai
Copy link
Contributor

kdmukai commented Aug 28, 2023

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
Copy link
Contributor

kdmukai commented Aug 28, 2023

ACK tested.

pytest had no complaints!

@jdlcdl
Copy link

jdlcdl commented Aug 30, 2023

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
Copy link

jdlcdl commented Aug 30, 2023

as of 3011211

ACK tested

@kdmukai
Copy link
Contributor

kdmukai commented Aug 31, 2023

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
Copy link

jdlcdl commented Aug 31, 2023

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
Copy link
Collaborator

ACK and Tested

@newtonick newtonick merged commit 39e7ebe into SeedSigner:dev Sep 1, 2023
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.

Remove parentheses from assert statements
4 participants