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

add copyright and authors to json report #22

Closed
wants to merge 19 commits into from

Conversation

notofug
Copy link
Collaborator

@notofug notofug commented Dec 29, 2023

This PR adds 'copyright' and 'authors' to the json output for use in post-processing to generate complete list of third-party components. I hope this is in line with your plan for the tool

Another thing I would like in the json is a 'pointer' to the associated downloaded license.html but that is not included here.

image

Copy link
Owner

@sensslen sensslen left a comment

Choose a reason for hiding this comment

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

Unfortunately I cannot let this go in as is.

  1. While the tests pass now, the functionality is not tested
  2. The tabular output does not provide the new information

@notofug
Copy link
Collaborator Author

notofug commented Jan 3, 2024

Latest commits addresses your comments.

But are you sure you want all properties in text-output also(?), it does not necessarily scale very well since it potentially make a very 'wide' output ;
image

Arguably the tab-output and json-output could be different : json is 'complete' while tab-output is only a summary of most relevant props - but I am not sure what is best way here.

@sensslen
Copy link
Owner

sensslen commented Jan 3, 2024

@notofug I'm fully aware of this and have been debating whether it's worth to add a command line flag that allows to enable/disable this. In general I'm not at all a fan of too many flags. I'm also not sure whether the tabular output should be moving towards a markdown compatible output which in turn will allow for the table to be rendered as html and thus avoiding the issues you describe.

If the tabular output does not provide all of the information I'd at least have this deviation documented in the readme so that it is obvious to users that the two do not provide equivalent information.....

LicenseInformationOrigin.Ignored)
})
.Using(new LicenseValidationResultValueEqualityComparer()));
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm missing a test with multiple authors here.... With AutoData you should be able to easily create an array of strings

@@ -21,13 +21,17 @@ public void SetUp()
new NuGetVersion(f.System.Semver()),
f.Internet.Url(),
f.Hacker.Phrase(),
null,
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please add tests for the filed formatting here as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had a go at this but have dont know how this stuff works so dont think I am right person for this ; otherwise believe I covered your requests

Copy link
Owner

Choose a reason for hiding this comment

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

I'll take a look when I get time....

@sensslen
Copy link
Owner

sensslen commented Jan 8, 2024

@notofug would you mind putting in some documentation on what is supported for which output variant?

@notofug
Copy link
Collaborator Author

notofug commented Jan 9, 2024

All output-variants are extended with 'Copyright' and 'Authors'. Should that be mentioned in the readme, or is it something else you have in mind?
Perhaps the readme should mention that the tool also processes packages.config and vcxproj's ?

@sensslen
Copy link
Owner

sensslen commented Jan 9, 2024

Oh, right - I forgot that you added that. I think it's a good idea to add some documentation about c++ project support. However this belongs in a different PR...

@notofug notofug force-pushed the print_copyright_and_authors branch from f05b4ba to c50a02c Compare January 29, 2024 07:06
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@notofug
Copy link
Collaborator Author

notofug commented Jan 29, 2024

OK that set off merge problems so now it doesnt work aparently. Maybe it is better i you cherry-pick it into your project so you get it as you prefer to have it?

@@ -22,6 +22,9 @@ public async Task Write(Stream stream, IList<LicenseValidationResult> results)
new ColumnDefinition("Version", license => license.PackageVersion, license => true, true),
new ColumnDefinition("License Information Origin", license => license.LicenseInformationOrigin, license => true, true),
new ColumnDefinition("License Expression", license => license.License, license => license.License != null),
new ColumnDefinition("Copyright", license => license.Copyright, license => license.Copyright != null),
new ColumnDefinition("Copyrigth", license => license.Copyright, license => license.Copyright != null),
Copy link
Owner

@sensslen sensslen Jan 29, 2024

Choose a reason for hiding this comment

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

@notofug
This is a problem... this adds a copyright column with a typo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, was fixed already but came in again in megamerge.
There are so many changes in this PR now that I dont really trust it to be correct or reviewable anymore so maybe better if you cherry-pick if you want to bring it into main?

Copy link
Owner

Choose a reason for hiding this comment

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

Most changes are expected output... if you ignore all .txt files, things are not bad anymore

@sensslen sensslen force-pushed the main branch 16 times, most recently from f54a3de to 7ca6e5d Compare February 26, 2024 22:33
@sensslen
Copy link
Owner

This PR has been superseeded by #50

@sensslen sensslen closed this Apr 10, 2024
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