Add stats executable for reporting repo state. #1541
Conversation
lib/sass_spec/stats/todo.rb
Outdated
| end | ||
| entry[:impl] = impl | ||
| entry[:issue] = match =~ /#/ ? match.split("#")[-1] : "?" | ||
| todos[path].push(entry) |
There was a problem hiding this comment.
Not necessarily something that needs to be fixed in this PR, but something that will probably need to be fixed at some point, is changing this to count the number of specs that are marked as TODO rather than counting the number of options files. A single options file at the root of a directory applies to every spec beneath that directory, so eventually you'll probably need to search for all tests (identified by input.{scss,css,sass} files) within the directory that contains the options.yml.
lib/sass_spec/stats/todo.rb
Outdated
| end | ||
| end | ||
|
|
||
| parser.on("--missing-issue", "Find todos that are missing a Github issue.") do |
There was a problem hiding this comment.
You can handle it manually by checking if options[:missing_issue] and options[:issue] are both set.
|
I addressed most comments; printing a nicely formatted report and specs are in flight. |
| unless issue =~ /.+#\d+/ | ||
| warn("Invalid issue.\n\n") | ||
| puts parser.help() | ||
| end | ||
| issue = issue.split("#") | ||
| number = issue.pop | ||
| issue = issue.join().split("/") | ||
| impl = issue.pop | ||
| options[:issue] = "#{impl}##{number}" |
There was a problem hiding this comment.
| unless issue =~ /.+#\d+/ | |
| warn("Invalid issue.\n\n") | |
| puts parser.help() | |
| end | |
| issue = issue.split("#") | |
| number = issue.pop | |
| issue = issue.join().split("/") | |
| impl = issue.pop | |
| options[:issue] = "#{impl}##{number}" | |
| unless issue =~ /([^/]+#\d+)$/ | |
| warn("Invalid issue.\n\n") | |
| puts parser.help() | |
| end | |
| options[:issue] = $1 |
| @options = options | ||
| end | ||
|
|
||
| ## |
There was a problem hiding this comment.
It's probably best to follow the doc comment style in the rest of the repo, which doesn't have this leading ##. I think you can omit the @param and @returns as well.
| if @options[:missing_issue] | ||
| metadata = filter_todos_missing_issue(metadata) | ||
| end | ||
| end |
There was a problem hiding this comment.
Is this supposed to return metadata as well?
| if @options[:todo] | ||
| metadata = filter_todos_by_implementation(metadata) | ||
| end |
There was a problem hiding this comment.
Style nit:
| if @options[:todo] | |
| metadata = filter_todos_by_implementation(metadata) | |
| end | |
| metadata = filter_todos_by_implementation(metadata) if @options[:todo] |
This will let you write what currently takes 11 lines in 3 without losing much readability 😃.
| def get_yaml(metadata) | ||
| dir = SassSpec::Directory.new(metadata.name) | ||
| YAML.load(dir.read("options.yml")) | ||
| end |
There was a problem hiding this comment.
Rather than re-reading the metadata here, maybe it would be better to have the Metadata object store the raw options map.
| metadata.select do |meta| | ||
| todos = meta.options[:todo] | ||
| next unless todos | ||
| todos.select { |todo| todo.include? @options[:todo] }.any? |
There was a problem hiding this comment.
Can you use Metadata#todo? here?
| # | ||
| # @return [Array<SassSpec::TestCaseMetadata>] | ||
| def get_metadata | ||
| dirs = @options[:dirs] || [SassSpec::SPEC_DIR].map do |dir| |
There was a problem hiding this comment.
| dirs = @options[:dirs] || [SassSpec::SPEC_DIR].map do |dir| | |
| dirs = (@options[:dirs] || [SassSpec::SPEC_DIR]).map do |dir| |
| end | ||
|
|
||
| option_files = dirs.flat_map do |dir| | ||
| dir.glob("**/options.yml").map do |file| |
There was a problem hiding this comment.
At this point, I think it would be pretty easy to make this find all test cases by making this:
| dir.glob("**/options.yml").map do |file| | |
| dir.glob("**/input.{scss,sass,css}").map do |file| |
Closes #1517