TreeRewriter::Enforcer

[marcandre]

This PR isolates conflict handling in an Enforcer class. A basic implementation with policty of :accept, :warn or :raise and a DiagnosticsEngine insures compatibility.

@iliabylich I didn't update the documentation much, I wanted to know if you thought this was a more worthwhile approach or than #15, or if it overcomplicates things.

[iliabylich]

Well, in general I like it (the only thing is that I don't really like half-abstract classes that are partially implemented), but what's important it seems to be a breaking change 😞 .

Yes, kwargs of Parser::Source::TreeRewriter are private and they are not documented at all, but:

1. https://github.com/yujinakayama/transpec/blob/master/lib/transpec/base_rewriter.rb#L11-L15
2. https://github.com/yujinakayama/gemologist/blob/master/lib/gemologist/abstract_gemfile.rb#L52-L57

^ and that's only what I've found by pulling reverse dependencies from rubygems.org (there can be more in non-gem github repos and I have no idea how to check them). I guess we have to keep interface of the constructor. Is it possible to keep it as is and still use a separate class?

Also, is this PR only about refactoring?

script to pull reverse deps

require 'open-uri'
require 'json'
require 'fileutils'

ROOT = File.expand_path('', __dir__)
REPOS_ROOT = File.join(ROOT, 'repos')
FileUtils.mkdir_p(REPOS_ROOT)

def get_json(url)
  JSON.parse(URI.open(url).read)
rescue
  {}
end

def download_gem(gem_name:, gem_uri:)
  if File.exists?("repos/#{gem_name}.gem")
    puts "Skipping wget repos/#{gem_name}.gem"
  else
    `wget #{gem_uri} -O repos/#{gem_name}.gem`
  end
end

def unpack_gem(gem_name)
  if File.directory?("repos/#{gem_name}")
    puts "Skipping gem unpack repos/#{gem_name}.gem"
  else
    `gem unpack repos/#{gem_name}.gem --target repos`
  end
end

deps = get_json('https://rubygems.org/api/v1/gems/parser/reverse_dependencies.json')

p deps

deps.sort.each do |gem_name|
  gem_info = get_json("https://rubygems.org/api/v1/gems/#{gem_name}.json")
  gem_uri = gem_info['gem_uri']

  download_gem(gem_name: gem_name, gem_uri: gem_uri)
  unpack_gem(gem_name)
end

[marcandre]

I should have been less lazy and document the whole thing; actually, kwargs of TreeRewriter are definitely public, and are supported. This adds an alternative enforcer argument, but if none is given, then other kwargs are still used to construct a Enforcer::WithPolicy which will act exactly as it did before.

Another way to see it is that TreeRewriter was both the public interface to construct rewriting actions and the enforcer. This decouples the "enforcer" behavior while maintaining compatibility as the constructor will build an enforcer for you if you don't supply one.

I could definitely make the base Enforcer not abstract by implementing ignore? as false too.

Given these, do you feel this is a worthwhile route?

[iliabylich]

Oh, right, I missed that there's still **kwargs in the signature.

Given these, do you feel this is a worthwhile route?

Yes, TreeRewriter is pretty big atm, but honestly I've never touched it (and IIRC nobody else except you 😄 ). So I guess it's up to you to decide.

I could definitely make the base Enforcer not abstract by implementing ignore? as false too.

I wasn't referring to raise NotImplementedError, I was pointing to the fact that base class is used for implementation inheritance. I'm not completely against it, and so the question is: does anyone need it? Do you expect anyone to create custom subclasses? If so it makes sense

[glitterblondie]

(Y)