search

eclipse-dash / dash-licenses

Extract license information from content.
http://projects.eclipse.org/projects/technology.dash
Eclipse Public License 2.0
48 stars 33 forks source link

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

Closed reissim closed 6 months ago

waynebeaton commented 6 months ago

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 commented 6 months ago

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.

reissim commented 6 months ago

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 commented 6 months ago

Merged! Thanks for the contribution.