-
Notifications
You must be signed in to change notification settings - Fork 17
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
Html rendering #47
base: master
Are you sure you want to change the base?
Html rendering #47
Conversation
.ruby-version
Outdated
@@ -0,0 +1 @@ | |||
ruby-2.4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't commit these .ruby-*
files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@segiddins
add to gitignore or just do not commit them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either is OK
cocoapods_acknowledgements.gemspec
Outdated
@@ -20,6 +20,8 @@ Gem::Specification.new do |spec| | |||
spec.add_runtime_dependency 'activesupport', '>= 4.0.2', '< 5' | |||
|
|||
spec.add_dependency "redcarpet", "~> 3.3" | |||
spec.add_dependency "nokogiri" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if at all possible, we really should avoid a dependency on nokogiri, since many have issues installing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@segiddins ok, html is too simple to use nokogiri.
cocoapods_acknowledgements.gemspec
Outdated
spec.add_development_dependency "bundler", "~> 1.3" | ||
spec.add_development_dependency "rake" | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@segiddins
maybe add 'rubocop' to development group to solve style questions?
@@ -0,0 +1,37 @@ | |||
module CocoaPodsAcknowledgements | |||
module Auxiliaries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
text | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing newlines missing in all these files, everything should also be using 2 spaces for indentation
@segiddins updated. |
Any chance of merging this branch ? |
I don't think it's done, see: https://github.com/CocoaPods/cocoapods-acknowledgements/pull/47/files#diff-86a5d812fd9ecac367ea96aaa1964994R4 |
@orta Update: |
Weird, build error.
|
def <= (html) | ||
self << html | ||
!self | ||
# self.content(html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented-out code
.gitignore
Outdated
xcuserdata/ | ||
|
||
# Pods | ||
Pods/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing newline
…dded. writers class method has been added.
class MarkdownParser | ||
class << self | ||
def markdown_parser | ||
@markdown_parser ||= Redcarpet::Markdown.new(Redcarpet::Render::HTML) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to pass render_options
to HTML render using user_settings
parameter of cocoapods-acknowledgements
plugin. Current implementation don't wrap http[s] links with a
HTML tag which makes them hard to style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krin-san How to retrieve user_settings
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I mistyped – it's named user_options
. This hash contains any custom parameters user might define in plugin inclusion string. E.g.:
plugin 'cocoapods-acknowledgements', :settings_bundle => true
We can use user_options
as a source of extra parameters for Redcarpet::Render::HTML
. Example:
plugin 'cocoapods-acknowledgements', :html_render_options => {:no_links => false, :link_attributes => {"link" => "#C0C0C0", "vlink" => "#808080", "alink" => "#FF0000"}}
Just need to figure out how to pass html_render_options
through HTMLGenerator
to MarkdownParser
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krin-san We can extend self.generate
method in Generator
to accept another parameter options
.
def generate_specs(target_description, sandbox, excluded, root_specs, options = {})
[]
end
def generate(target_description, sandbox, excluded, options = {})
root_specs = target_description.specs.map(&:root).uniq.reject {|spec| excluded.include?(spec.name)}
return nil if root_specs.empty?
generate_specs(target_description, sandbox, excluded, root_specs)
end
I don't see any better option here.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a Ruby developer actually 😄. Whichever option allowing to define render_options
in Podfile
plugin inclusion line looks fine for me.
@segiddins Could you post a review? |
Architecture changed.