V2: I have a working version with crass, Css parsing imporved #156
Conversation
stoivo
changed the title
|
I already rebased v2 on master to make this easier to read, but the diff here did not change, can you try and rebase again ? |
| case node | ||
| in node: :style_rule |
There was a problem hiding this comment.
can we use this instead:
case node[:node]
when :style_rule
(and then break down further based on name inside of these)
There was a problem hiding this comment.
We can but why do you think this is better? I like the pattern matching code
There was a problem hiding this comment.
either way is fine, was mostly thinking we could have 1 case for every type and then some method that handles each sub-type and that might be cleaner
| declarations = create_declaration_from_properties(node[:children]) | ||
|
|
||
| add_rule_options = { | ||
| selectors: node[:selector][:value], | ||
| block: declarations, | ||
| media_types: current_media_queries | ||
| } | ||
| if options[:capture_offsets] | ||
| add_rule_options.merge!( | ||
| filename: options[:filename], | ||
| offset: node[:selector][:tokens].first[:pos]..node[:children].last[:pos] | ||
| ) | ||
| end | ||
|
|
||
| add_rule!(**add_rule_options) |
There was a problem hiding this comment.
would it make sense to have each of these blocks be a method ?
There was a problem hiding this comment.
I would like to try to keep the main functionality here in add_block! but maybe I should do some work here to extract some more helper function. I will look at it in later PR if that's okay?
There was a problem hiding this comment.
sounds good, was mostly hard to keep all the structure in mind when reviewing/reading :)
| assert_equal file_name, rules[1].filename | ||
| end | ||
|
|
||
| # http://github.com/premailer/css_parser/issues#issue/4 | ||
| def test_capturing_offsets_from_remote_file | ||
| # TODO: test SSL locally | ||
| # TODO: cache request to make test not require internet (and so much faster) |
There was a problem hiding this comment.
add webmock so that will just break and we return stubbed replies ?
There was a problem hiding this comment.
I think webmock here is the right choice too.
| in node: :whitespace # nothing | ||
| in node: :semicolon # nothing | ||
| in node: :error # nothing |
There was a problem hiding this comment.
can we remove these 3 or even better add a "else raise" ?
There was a problem hiding this comment.
This is pattern patching. If none of the branches matches it will raise NoMatchingPatternError. I like that error because adds which node it received
One example
NoMatchingPatternError: {:node=>:style_rule, :selector=>{:node=>:selector, :value=>"body", :tokens=>[{:node=>:ident, :pos=>6, :raw=>"body", :value=>"body"}, {:node=>:whitespace, :pos=>10, :raw=>" "}]}, :children=>[{:node=>:whitespace, :pos=>12, :raw=>" "}, {:node=>:property, :name=>"font-size", :value=>"10pt", :children=>[{:node=>:whitespace, :pos=>23, :raw=>" "}, {:node=>:dimension, :pos=>24, :raw=>"10pt", :repr=>"10", :type=>:integer, :unit=>"pt", :value=>10}, {:node=>:whitespace, :pos=>28, :raw=>" "}], :important=>false, :tokens=>[{:node=>:ident, :pos=>13, :raw=>"font-size", :value=>"font-size"}, {:node=>:colon, :pos=>22, :raw=>":"}, {:node=>:whitespace, :pos=>23, :raw=>" "}, {:node=>:dimension, :pos=>24, :raw=>"10pt", :repr=>"10", :type=>:integer, :unit=>"pt", :value=>10}, {:node=>:whitespace, :pos=>28, :raw=>" "}]}]}
I think I need to add some more section for stuff like @keyframe
There was a problem hiding this comment.
oh nice, did not realize they added that ✨
stoivo
force-pushed
the
v2
branch
2 times, most recently
from
ebe56f6
to
806f91c
Compare
second argument to assert_raises is custom error message
Using ArgumentError sounds like a good idea since we received invalid argument to this method, but this exception is suppose to be when you pass the wrong number of arguments or missing keywords.
Error Your bundle only supports platforms ["arm64-darwin-22", "java"] but your local
I created a new v2 branch and good in some more of the commits on master. Maybe we can make this the base for v2. I have more changes we should do before releasing but you can have a look and give tel me what you think?
One of the things I would like to do is to remove depreciation and remove unused things from regexps.rb