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

Fixed lint and unit test issues. #150

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

liamgm
Copy link

@liamgm liamgm commented Sep 10, 2024

Summary of Changes

Fixed all coding style complaints raised by laravel/pint against current head 83ad0be

Fixed unit test after confirming public key string calculated by phpseclib3 is correct against python's cryptography library.

Did not fix coverage tests as I don't have a CC secret, but pcov is locally satisfied.

No functional changes, just prepping to address issue 149.

Testing

  • I have added automated tests for my changes (no changes, but existing unit test was fixed)
  • I ran composer test before opening this PR
  • I ran composer lint-fix before opening this PR

Confirmed this is correct publickey using python crytpography library
@liamgm liamgm requested a review from dbhynds as a code owner September 10, 2024 22:55
@liamgm
Copy link
Author

liamgm commented Sep 10, 2024

FWIW: I disagree with the most common lint failure in there that arises from this ambivalent decision.

If the maintainers agree with the alternate convention to require the empty parentheses on class instantiations without parameters, I can amend this to instead set that preference in pint.json

@liamgm
Copy link
Author

liamgm commented Sep 11, 2024

I think this is a much nicer commit: I can create a new PR if this patch is preferable.

Copy link
Member

@dbhynds dbhynds left a comment

Choose a reason for hiding this comment

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

FWIW: I disagree with the most common lint failure in there that arises from laravel/pint#284.
If the maintainers agree with the alternate convention to require the empty parentheses on class instantiations without parameters, I can amend this to instead set that preference in pint.json

I appreciate / respect the difference of opinion. We are currently using this standard set of linting rules across multiple other projects, so I'd like to keep them consistent, rather than modifying the pint.json on this project. Either way, thank you for the PR fixing these!

@dbhynds dbhynds merged commit bb8b3d0 into packbackbooks:master Sep 11, 2024
4 of 5 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.

2 participants