Skip to content

Conversation

@nanobowers
Copy link
Contributor

Resolves issue #8 by adding rake-tasks to get XKCD and CSS4 colors from the URIs listed in the documentation.

It is worth noting that:

  1. Parsing the xkcd color file is pretty easy, just tab-separated values
  2. Parsing the css4 draft webpage is harder and so a development dependency of Nokogiri was added to scrape through the webpage. Since this is a draft webpage, it is likely this scraping code may no longer work if the HTML tables are modified.
  3. There was an intermediate commit of the data which split the colors/color_data.rb into separate files, in preparation for some portions (xkcd.rb, css4.rb) to be autogenerated.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

Thanks!

@ericgpks you may be interested in this.

}.freeze
end
end
require_relative "./color_data/matplotlib"
Copy link
Member

Choose a reason for hiding this comment

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

We don't need the preceding ./:

Suggested change
require_relative "./color_data/matplotlib"
require_relative "color_data/matplotlib"

require "open-uri"
require "nokogiri"

module GetColors
Copy link
Member

Choose a reason for hiding this comment

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

How about CSS4ColorDataGenerator?


class CSS4
def initialize
@colordata = []
Copy link
Member

Choose a reason for hiding this comment

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

We can simplify this to @data.

@colordata = []
end

def get_data
Copy link
Member

Choose a reason for hiding this comment

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

How about parse?

doc = Nokogiri::HTML(html)
table = doc.css("table.named-color-table").first
table.css("tbody").children.each do |kid|
colorname = kid.css("dfn").text.strip
Copy link
Member

Choose a reason for hiding this comment

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

We can simplify this to name.

end

def dump_file(filename)
File.open(filename, "w") do |fh|
Copy link
Member

Choose a reason for hiding this comment

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

Opened file is a File object not file handler. So file is better than fh:

Suggested change
File.open(filename, "w") do |fh|
File.open(filename, "w") do |file|

CSS4FOOTER
end

def dump_file(filename)
Copy link
Member

Choose a reason for hiding this comment

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

How about generate?

end

def header(fh)
fh.puts <<~CSS4HEADER
Copy link
Member

Choose a reason for hiding this comment

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

We can remove CSS4 prefix because this class is for CSS4:

Suggested change
fh.puts <<~CSS4HEADER
fh.puts <<~HEADER

css4.dump_file("lib/colors/color_data/css4.rb")
end

desc "Get color data files from the web, per issue #8"
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to refer #8 here.

Suggested change
desc "Get color data files from the web, per issue #8"
desc "Get color data files from the web"

end

desc "Get color data files from the web, per issue #8"
task :get_colors => [:get_xkcd, :get_css4]
Copy link
Member

Choose a reason for hiding this comment

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

How about colors:generate?

Suggested change
task :get_colors => [:get_xkcd, :get_css4]
namespace :colors do
namespace :generate do
task :xkcd do
# ...
end
task :css4 do
# ...
end
end
task :generate => [:xkcd, :css4]
end

@kou
Copy link
Member

kou commented Jan 26, 2022

Could you create a separated pull request that separates colors/color_data.rb to colors/color_data/*.rb?

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.

2 participants