kessler / license-report

create a short report about a project's dependencies (license, url etc)
MIT License
226 stars 39 forks source link

feat: add support for optional field 'installedFrom' #75

Closed coyoteecd closed 2 years ago

coyoteecd commented 2 years ago

This PR adds a "downloadLink" field that displays the "resolved" URL for each package, as extracted from package-lock.json. Some legal departments require the "source" the package was downloaded from.

For example, https://registry.npmjs.org/buffer/-/buffer-4.9.2.tgz

BePo65 commented 2 years ago

As we all know that we should not discuss with legal it is probably a good idea to add this field :-) I think this is an excellent pr, it even includes the updated tests that are hard work to do.

With my heavy focus on CleanCode I would like to discuss about some of the chosen names for variables and functions. My ideas are:

  1. in 'config.js' we could rename 'DownloadLink' to 'sourceLink' and rename existing field 'link' to 'repositoryLink' (to differentiate the 2 'links' - breaking change!) or what I would prefer rename 'DownloadLink' to 'source' (and keep 'link'; no breaking change)

    In any case; the name of the new field must be camelCase to be consistent with the other fields.

  2. in 'getInstalledVersions.js' rename the variable 'installedVersions' to 'installedPackagesData' (as this is what we extract from the file) rename the variable 'installedVersion' to 'installedPackageData'

    rename the file 'getInstalledVersions' to 'getInstalledPackagesData', as the function returns more than just the 'installed versions'. This renaming should be done for the file and for the variable in index.js.

  3. in 'getPackageReportData.js' Problem: if we use the names 'installedVersion' and 'installedVersionDownloadLink', one time we use 'version' as a synonym for 'version number' and one time as a synonym for 'package'; this is not consistent. To make the names more consistent, we could: rename 'installedVersion' to 'installedPackageVersion' and rename 'installedVersionDownloadLink' to 'installedPackageSource' (or what we did to 'DownloadLink' in 1.) or what I like more keep 'installedVersion' and rename 'installedVersionDownloadLink' to 'source' (or what we did to 'DownloadLink' in 1.)

    and in the returned object: rename 'downloadLink' to 'source' (or what we did to 'DownloadLink' in 1.)

  4. consider the change from 1. to all the tests and test fixtures (sorry for that :-)

What do you think about all that?

BTW: I had to update the packages as a result of the last dependabot report. So it would be nice, if you could rebase this pr to the latest commit of master.

coyoteecd commented 2 years ago

@BePo65 Could you use the inline review comments feature in GitHub (switch to File Changes tab and add them to the related changes). This leaves room for discussing them individually. For now, I will reply below

As we all know that we should not discuss with legal it is probably a good idea to add this field :-) I think this is an excellent pr, it even includes the updated tests that are hard work to do.

With my heavy focus on CleanCode I would like to discuss about some of the chosen names for variables and functions. My ideas are:

1. in 'config.js' we could
   rename 'DownloadLink' to 'sourceLink' and rename existing field 'link' to 'repositoryLink' (to differentiate the 2 'links' - breaking change!)
   or what I would prefer
   rename 'DownloadLink' to 'source' (and keep 'link'; no breaking change)
   In any case; the name of the new field must be camelCase to be consistent with the other fields.

First, downloadLink is already camelCase (this is where inline reviews help). Second, I don't mind the change, but imo source is a bit unclear, is it a link, and if so, what's different from existing link. How about installedFrom? Lines up well with installedVersion and no breaking changes...

2. in 'getInstalledVersions.js'
   rename the variable 'installedVersions' to 'installedPackagesData' (as this is what we extract from the file)
   rename the variable 'installedVersion' to 'installedPackageData'
   rename the file 'getInstalledVersions' to 'getInstalledPackagesData', as the function returns more than just the 'installed versions'.  This renaming should be done for the file and for the variable in index.js.

Yeah I wanted to avoid submitting way too many changes since some maintainers are not open to merging those. If you're okay with accepting bigger changes I will follow with other features that we need in our project.

I will do the renames.

3. in 'getPackageReportData.js'
   Problem: if we use the names 'installedVersion' and 'installedVersionDownloadLink', one time we use 'version' as a synonym for 'version number' and one time as a synonym for 'package'; this is not consistent. To make the names more consistent, we could:
   rename 'installedVersion' to 'installedPackageVersion' and
   rename 'installedVersionDownloadLink' to 'installedPackageSource' (or what we did to 'DownloadLink' in 1.)
   or what I like more
   keep 'installedVersion' and
   rename 'installedVersionDownloadLink' to 'source' (or what we did to 'DownloadLink' in 1.)
   and in the returned object:
   rename 'downloadLink' to 'source' (or what we did to 'DownloadLink' in 1.)

Will do

4. consider the change from 1. to all the tests and test fixtures (sorry for that :-)

Feedback: the tests were a pain to modify (not fun to figure out why a large single-line html string does not equal another one) I would suggest using less data, or formatting it in a friendlier way.

What do you think about all that?

BTW: I had to update the packages as a result of the last dependabot report. So it would be nice, if you could rebase this pr to the latest commit of master.

coyoteecd commented 2 years ago

@BePo65 I force-pushed a commit with the renames (see above).