3.0.0 complete rewrite #59
Conversation
@jeena asked a very reasonable question about the differences between v2 and v3, attempting to enumerate those differences.
…the parser, or marked them as pending
…r versions of ruby
veganstraightedge
left a comment
There was a problem hiding this comment.
I left the same comment in a few places, but it can be applied everywhere. I think this is a good time to move to almost/all named parameters since it's already a big breaking change release.
Also, I don't love the test suite requiring npm. I understand why you did it. It's pragmatic. But I'd like to see this get moved over to a ruby/rake workflow in the future.
lib/microformats2/format_parser.rb
Outdated
| when node.is_a?(Nokogiri::XML::Element) then [parse_for_microformats(node, parsing_children)] | ||
| end | ||
| end | ||
| def parse(element, base=nil, element_type=nil, fmt_classes=[], backcompat=false) |
There was a problem hiding this comment.
I think you should use named parameters here.
lib/microformats2.rb
Outdated
| def parse(html) | ||
| Parser.new.parse(html) | ||
| def parse(html, base: nil) | ||
| Parser.new.parse(html, base) |
There was a problem hiding this comment.
I think Parser.new.parse() should use a named parameter for base.
lib/microformats2/parser.rb
Outdated
| end | ||
|
|
||
| def parse(html, headers={}) | ||
| def parse(html, base= nil, headers={}) |
There was a problem hiding this comment.
More that should be named parameters.
| class Collection | ||
| def initialize(hash) | ||
| @hash = hash | ||
| end |
There was a problem hiding this comment.
These methods should have vertical whitespace between them.
| end | ||
| def to_hash | ||
| @hash | ||
| end |
There was a problem hiding this comment.
I think it'd be good to alias to_h to to_hash too.
There was a problem hiding this comment.
This is fine, but for future resilience and DRYer code, you should use alias_method.
alias_method :baz, :foo
missed a few tabbing issues more consistant spacing changed all functions to be named parameters after the first this should have been changed when that change was first made, but this now adds a min version for ruby of 2.0 when named params first introduced
hmm thought i had privileges to review, it seems not.