Should RDF::Changeset even be an RDF::Mutable?

[doriantaylor]

Pursuant to discussion in issues #403 and #404, I am putting up this issue as a marker for work on potentially disentangling RDF::Changeset from RDF::Mutable, since it fails a number of the mutable tests from rdf-spec, and will not pass them without adding a bunch of code whose sole purpose would be to pass tests, and removing other code that had been put there on purpose.

Notwithstanding, the inserts and deletes members of an RDF::Changeset instance may indeed themselves make sense behaving as RDF::Mutable objects. As such I propose to explore the optimal configuration for RDF::Changeset.

[gkellogg]

@no-reply you originally authored this bit, so you may have an opinion.

[doriantaylor]

TL;DR:

• RDF::Changeset looked like (via the docs) it ought to accept BGPs/incomplete triples on the delete side
• indeed it does, with only a minor change
• it turned out, separately, that RDF::Repository::Implementation assumed constant RDF::Statements in its apply_changeset method so this was also changed
• the question was raised as to whether the include of RDF::Mutable even belongs in RDF::Changeset

[doriantaylor]

Upon further inspection, it looks like the major contributions to RDF::Changeset from RDF::Mutable are the coercions on #insert (via RDF::Writable) and #delete.

A candidate for decoupling RDF::Changeset from RDF::Mutable is simply to pop out this coercion code into its own protected method, like so:

  def process_statements(statements, query: false, constant: false, &block)
    raise ArgumentError, 'expecting a block' unless block_given?

    statements = statements.map do |value|
      case
      when value.respond_to?(:each_statement)
        block.call(value)
        nil
      when (statement = Statement.from(value)) &&
          (!constant || statement.constant?)
        statement
      when query
        block.call(query(value))
        nil
      else
        raise ArgumentError, "Not a valid statement: #{value.inspect}"
      end
    end.compact

    block.call(statements) unless statements.empty?

    # eh might as well return these
    statements
  end

Calling it from RDF::Writable looks like this:

    def insert(*statements)
      process_statements(statements) { |value| insert_statements(value) }

      return self
    end

…while calling it from RDF::Mutable looks like this:

    def delete(*statements)
      raise TypeError.new("#{self} is immutable") if immutable?

      process_statements(statements, query: true, constant: true) do |value|
        delete_statements(value)
      end

      return self
    end

RDF::Changeset would likely then only need to include RDF::Writable and not RDF::Mutable, and would just need its own #delete method. At any rate, this change eliminates some duplicate code as-is, and passes the test suite.

[gkellogg]

That seems reasonable to me, in which case it should probably be able to run the it_behaves_like 'an RDF::Writable' specs.

Why use a constant parameter at all, and not just depend on statement.constant??

[doriantaylor]

I tried to make that method as generic as possible; the part of RDF::Writable#insert it replaces does not fuss over whether or not the statement is constant, however RDF::Mutable#delete does.

Although now I'm thinking it may not even necessarily make sense for RDF::Changeset to include RDF::Writable either, because again it fails a bunch of tests that would entail writing a bunch of nonsensical methods that would only exist to satisfy the tests. In such a case I would recommend moving the coercion stuff to a new module e.g. RDF::Util::Coercions, or otherwise keep it in RDF::Writable and simply have RDF::Changeset#insert and #delete proxy to its members which I would change to implement RDF::Mutable.

Eager for any insight from @no-reply.

[no-reply]

I can't recall the history of this (it looks like the original include is @gkellogg's at 22d6fea51802b), but my guess is that this module include was more about convenient code reuse and not an attempt to conform to a principled type system.

I agree that Changeset is really neither Mutable nor Writable, so I'm :+1: on anything that maximizes code reuse while removing them from its dependency tree. Some care should be taken to ensure that any methods they provided that are/were useful for Changeset, however incidentally, aren't removed summarily.

As a first step: what about just def mutable?; false; end and def writable?; false; end?

[gkellogg]

I didn't write all that stuff, even though my name's on the commit. Probably needed to do some massaging.

[doriantaylor]

Thanks; my original concern was to be able to put wildcards (ie query patterns) into the deletes of an RDF::Changeset, which seems to work but appeared to depend implicitly on being hooked by code inRDF::Mutable. That said, it might work anyway if all of that was ripped out. Again, the only thing that comes from those modules appears to be the business around coercing the input from #insert and #delete into statement-ish objects.

Anyway I think I have an idea for how to proceed.