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

chore: refactor for removing pdb symbols #330

Merged
merged 2 commits into from Dec 7, 2023
Merged

Conversation

peppelinux
Copy link
Contributor

@peppelinux peppelinux commented Aug 23, 2023

This PR represents an easy refactor of small parts of the source code where the PDB symbols was used for variable assignments. It simply removed the variable names a, c that are used in PDB to inspect the args and continue the execution.

Closes #327 (comment)

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

This is good, just some minor nits on the chosen names, but otherwise fine.
Please add your sign-off tag to the commit.

jwcrypto/jwa.py Outdated Show resolved Hide resolved
jwcrypto/jwk.py Outdated Show resolved Hide resolved
jwcrypto/jwk.py Show resolved Hide resolved
jwcrypto/jws.py Outdated Show resolved Hide resolved
jwcrypto/tests.py Outdated Show resolved Hide resolved
@simo5
Copy link
Member

simo5 commented Nov 28, 2023

@peppelinux any chance you can fix the nits and push anew? I'd like to merge this.

@peppelinux
Copy link
Contributor Author

Ciao @simo5
I just have pushed my commit with your revision, thank you for the reminder, I just needed that!

@simo5
Copy link
Member

simo5 commented Nov 29, 2023

@peppelinux great, would you mind squashing the commits together, I will then merge.

@simo5
Copy link
Member

simo5 commented Nov 29, 2023

Uhmm sounds like the linter is not happy, if "aa" does not work, feel free to change to "aad"

@peppelinux
Copy link
Contributor Author

Done @simo5 !

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

Unfortunately the linter is still not 100% happy, 4 places need to change ai->aad

jwcrypto/jwa.py Outdated Show resolved Hide resolved
@peppelinux peppelinux requested a review from simo5 December 7, 2023 11:44
Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution

@simo5 simo5 merged commit 6ee0e89 into latchset:main Dec 7, 2023
12 of 13 checks passed
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.

variables naming convention
2 participants