Compromise Interface Redesign

[willow-ahrens]

As discussed in our meeting, this PR simply removes exprhead. Because the documentation of similarterm(x, head, args) currently says x is allowed to be a type, I believe that correct implementations of similarterm already need to follow exactly the same semantics that we proposed for maketerm in our discussion. Thus, I have just made that clearer by specifying that implementers must overload similarterm(T::DataType, head, args) and providing a simple ``similarterm(x, head, args) = similarterm(T::DataType, head, args)` definition. I don't think the rename to maketerm is necessary.

@shashi @0x0f0f0f

[0x0f0f0f]

Nice

[willow-ahrens]

@shashi I'm not sure I understand what precise changes you wish to see, but I'm sure I would agree with them, could you go ahead and make the changes and commit them to this branch? After that I think we could merge

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 46.66667% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 34.78%. Comparing base (f43e305) to head (736fe48).
Report is 4 commits behind head on master.

Files	Patch %	Lines
src/TermInterface.jl	0.00%	5 Missing ⚠️
src/utils.jl	0.00%	2 Missing ⚠️
src/expr.jl	87.50%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   42.85%   34.78%   -8.08%     
==========================================
  Files           3        3              
  Lines          28       23       -5     
==========================================
- Hits           12        8       -4     
+ Misses         16       15       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shashi]

Ok, so reading Alessandro's comment makes me think that we still need 2 different sets of accessors to get this going:

• head, children
• operation, arguments. --> works only for function call expressions

head(x::Expr) = x.head
head(t::Term) = Term # Term type itself!
children(t::Term) = [t.f, t.args...]

I think we need a constructor which will be taking head and children and not operation and arguments. So I am thinking I will just make a maketerm and deprecate similarterm. similarterm can be implemented in terms of maketerm where head is :call or Term in the above cases.

@willow-ahrens I hope this makes sense. Basically operation arguments will continue to work as before, and head and children will be exactly as your current proposal, we will not have exprhead or is_call as a kwarg to the constructor. We get what we all want.

I will introduce a iscall trait: iscall(t::Term) = true; iscall(ex::Expr) = ex.head == :call, and then define the fallback operation(t) = iscall(t) ? children(t)[1] : error("not a call expression")

What's here is easy to make backwards compatible with the right deprecations.

@0x0f0f0f please try to keep your comments more concise, so you can get quicker responses :-p

[0x0f0f0f]

@shashi can you please push a version with passing tests? i will reply consistently today after work. i like the latest proposal, i have some comments but i think we are coming to a conclusion :)

[willow-ahrens]

A few small changes

[0x0f0f0f]

Ok, I think we're finally coming to a conclusion!

[0x0f0f0f]

@shashi I could push some fixes later this evening if you're on low bandwidth

[0x0f0f0f]

Pushed some adjustments

[0x0f0f0f]

Finalized some adjustments. I think we can merge now. WDYT @shashi @willow-ahrens ?

[shashi]

It's looking good.

[0x0f0f0f]

Ready to merge

[willow-ahrens]

if similarterm is deprecated anyway, should it take an exprhead kwarg? I'm not in favor of exprhead, but I wonder if that would keep things working that used it. Apart from that one thought, this looks great to me let's merge it.

[0x0f0f0f]

if similarterm is deprecated anyway, should it take an exprhead kwarg? I'm not in favor of exprhead, but I wonder if that would keep things working that used it. Apart from that one thought, this looks great to me let's merge it.

Since we'e doing a major version bump and the package currently has no dependents i would even remove similarterm and just move it to the next SymbolicUtils version that is going to use TI 0.4.0.

[willow-ahrens]

I'm in favor of that, how about @shashi ?

[willow-ahrens]

This is supposed to be a minimal interface, so that would make sense

[shashi]

Ok that is fine with me.

[0x0f0f0f]

Merging and releasing

[nsajko]

maketerm(T, head, children, type=nothing, metadata=nothing)

Constructs an expression. [...], type is the type of the S-expression.

What is the meaning of the type argument here? "Type of the sexp" means nothing to me, sexps don't have type?

[0x0f0f0f]

maketerm(T, head, children, type=nothing, metadata=nothing)
Constructs an expression. [...], type is the type of the S-expression.

What is the meaning of the type argument here? "Type of the sexp" means nothing to me, sexps don't have type?

I think the doc should be improved. It represent the type of things in typed lambda calculi. It can be redundant with metadata, but it's planned to be used as symtype in SymbolicUtils.

[nsajko]

xref #36