Skip to content

Conversation

@olleolleolle
Copy link
Collaborator

@olleolleolle olleolleolle commented Nov 4, 2017

This PR carries commits mentioned in #16.

Questions for reviewer:

  • This is a start (Update: I've now taken over the code which was "pickupable")
  • What test cases can we modify to cover the new text patterns?
  • Do we wish to have a purpose-built dependency_manifest.yml codepath in the gem, or can we offer before and after hooks in the Rake task, to offer manipulability of the licenses information gathered?

Things that stand out:

  • ENV['DEBUG'] to enable "debug mode" could become a logger instead Replaced.
  • Extract the dependency_manifest things to its own PR

@olleolleolle olleolleolle requested a review from dblock November 4, 2017 22:17
Copy link
Owner

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Not sure what tests should be modified, but tests should be modified ;)

Maybe separate the whole dependency manifest stuff into a different PR?

Needs documentation, changelog, etc.

@@ -1 +1,10 @@
require 'gem_licenses'

if defined?(Rails)
Copy link
Owner

Choose a reason for hiding this comment

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

Should live in its own file and be loaded via the above require. See how other gems do this.


# manually clean up some cruft
after = before.collect do |license|
stringified = license.to_s
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpicking, but this could be a when with all three values an license.to.s_upcase.


def log_warning(message)
warn message if debugging?
end
Copy link
Owner

Choose a reason for hiding this comment

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

As you mention this should be a logger.

@olleolleolle
Copy link
Collaborator Author

@dblock Thanks for looking at it - any kind of feedback about this is great.

  • Do we wish to have a purpose-built dependency_manifest.yml codepath in the gem, or can we offer before and after hooks in the Rake task, to offer manipulability of the licenses information gathered?

I've now realized that Gem.licenses is the method that such an interested user can make use of. I should write an example for the README, how to do stuff with Gem.licenses in a custom-written Rake task for your CI.

@dblock
Copy link
Owner

dblock commented Nov 6, 2017

Remaining things are logger (getting rid of debugging?), changelog/readme.

@olleolleolle
Copy link
Collaborator Author

olleolleolle commented Nov 6, 2017 via email

@dblock
Copy link
Owner

dblock commented Nov 6, 2017

I am still half confused about to_dependency_manifest.rb btw, so maybe split that into a different PR?

@olleolleolle
Copy link
Collaborator Author

@dblock I made the simple decision to follow the logging/output style of the parent class Gem::Specification - use warn to output warnings, and have no other output.

@olleolleolle
Copy link
Collaborator Author

(That's it, for tonight. I'm out of time. See you soon!)

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.

3 participants