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

Ensure that nuget dependency path exists before reading #280

Merged
merged 2 commits into from
Jun 2, 2020

Conversation

jonabc
Copy link
Contributor

@jonabc jonabc commented Jun 1, 2020

A corollary to #273 to check that the path to a nuget dependency exists before calling File.read. This is separated from the referenced PR as that is waiting on some feedback from the dotnet team and I'd like to put out a new release soon.

Besides making the ☝️ aforementioned fix, this also slightly changes the functionality of the dependency to be just a bit more performant when:

  1. not using the cache command
  2. using the cache command when no changes are needed to cached metadata

These make up the broad majority of licensed runs, so hopefully this makes things a little bit faster. Two specific changes here:

  1. Don't get the project_url or description until evaluating a dependency's license_metadata. That function is only called when caching new metadata or overwriting existing metadata, and takes reading and parsing nuspect_path out of the common path.
  2. Move checks on nuspec_path and nuspec_contents into each cached begin/end block. This has the effect of caching a nil value rather than not caching a value + re-porforming those checks when they evaluate to false.

/cc @paveliak @zarenner FYI on parceling out some of the fix from the referenced PR

super.tap do |record_metadata|
record_metadata["homepage"] = project_url if project_url
record_metadata["summary"] = description if description
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I wanted to do something like this but I think wasn't sure if it was cool to override license_metadata. Also TIL about tap, thanks for the knowledge 👍

Copy link

Choose a reason for hiding this comment

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

Thinking of it in terms on C# is it akin to overriding virtual property? That is:

  1. Get property value from base class
  2. Assign two metadata values
  3. Return modified property

Copy link
Contributor Author

@jonabc jonabc Jun 2, 2020

Choose a reason for hiding this comment

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

@paveliak yup 👍 . In ruby all methods can be overridden by default. There is no special syntax like virtual and override which explicitly tell which methods can be overridden and which methods are overriding base classes. The call to super here is equivalent to calling base.licensed_metadata() in c#.

@jonabc jonabc merged commit bedeab0 into master Jun 2, 2020
@jonabc jonabc deleted the check-nuget-dependency-path branch June 2, 2020 00:11
@jonabc jonabc mentioned this pull request Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants