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

Adding feature to scan pnpm-lock.yaml (PNPM) #339

Closed
wants to merge 3 commits into from

Conversation

reissim
Copy link
Contributor

@reissim reissim commented Apr 21, 2024

No description provided.

@waynebeaton waynebeaton self-assigned this Apr 21, 2024
@waynebeaton
Copy link
Collaborator

Thanks for this...

Out of curiosity... I feel that I've not done a great job with the file format reader tests. I see that you've copied what I've done... do you have any thoughts on how we might do better, more comprehensive tests in this context?

@waynebeaton
Copy link
Collaborator

I assume that the copyright headers on PnpmPackageLockFileReader.java and the test class are because you copied existing files. But... when I do a diff on the files, it seems that the content has been pretty extensively changed and should be regarded as new content.

Unless I've missed something, the copyright should be assigned to either you or your employer, and the year should be 2024.

Did I miss something? If not, please update the copyright headers.

Updated description
@reissim
Copy link
Contributor Author

reissim commented Apr 22, 2024

Thanks for this...

Out of curiosity... I feel that I've not done a great job with the file format reader tests. I see that you've copied what I've done... do you have any thoughts on how we might do better, more comprehensive tests in this context?

Comprehensive tests would be to have all possible (edge) cases which I hope we have. I checked the unit test which they are using in the PNPM Repo but to be honest I didn't find all cases there also the reason I used a pnpm-lock.yaml file generated from a real project like you used from a Eclipse Project "Ditto Clients". Only idea would be to test it against a Schema File or even generate the pattern out of it if it exist for the File Reader don't know if thats too much though? I know there exist some for NPM but for PNPM there is none for example.

I think the tests are fine, guess there is no easy (automatic) way to find all edge cases then to test against a real file, get
rid of all errors and make sure we catched all libs.

I assume that the copyright headers on PnpmPackageLockFileReader.java and the test class are because you copied existing files. But... when I do a diff on the files, it seems that the content has been pretty extensively changed and should be regarded as new content.

Unless I've missed something, the copyright should be assigned to either you or your employer, and the year should be 2024.

Did I miss something? If not, please update the copyright headers.

Thanks for the review, Wayne!

Done!

@waynebeaton
Copy link
Collaborator

Merged! Thanks for the contribution.

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