Compromise: Consistent interface

[creatorrr]

Just wanted to get your feedback on API consistency. I think we could do a lot more to make the API consistent and intuitive to use. Some examples:

• to_past, to_present, to_past functions return a mutated wrapped instance but americanize and britishize return just the changed text. Is that by design?
• Sentence.to_past returns the mutated Sentence instance but same doesn't apply for Text.to_past. (I don't know if there's any way to fix that other than extending Array.prototype)
• Unlike other wrappers, (new NLP.term(...)).text is a string property rather than a function.

Also, I think it might make things a lot less error-prone if we can make the entire library immutable (that'd break backwards compatibility, maybe offer an immutability helper?)

[spencermountain]

completely agree. i think all methods should start reliably returning this, so they can be all chained and more consistent. the .text vs .text() thing also kills me all the time. i can't decide which one to move. it should definitely be fixed.

a lot of smart people are doing immutability now, and it's cool. Besides squashing bugs, what sort of features in nlp would it allow? 'undo' and things? If you parsed a novel, would it slow it down much? what's an immutability helper?

[creatorrr]

i think all methods should start reliably returning this

Agreed. That's a great idea.

the .text vs .text() thing also kills me all the time

Hmm, yeah. I think text() is probably a better idea, esp if a plugin wanted to overwrite it and do some postprocessing. It should be consistent but it's a but difficult as it'd be a breaking change. This is a dirty hack but it could make do:

Term.prototype.text = function text() {
  // ...does whatever here and returns text
}

Term.prototype.text.toString = function textToString() {
  return this.text();
}

// Example
let t = NLP.term("hello");

t.text()  // -> hello
t.text  // -> [Object Function]

// However, in any string cast
t.text+""  // -> hello

Idk, but maybe something like that could preserve some backwards compatibility.

Besides squashing bugs, what sort of features in nlp would it allow?

Well, I can't think of an immediate example but mainly immutability would allow for a better sense of thinking about nlp transforms and making mutations transparent in the code. For example:

// Current API
let myText = NLP.text("she likes reading");

// ... at some point
myText.to_past();  // Mutates myText
myText.text()  // -> she liked reading

// At some other point in the code,
// it's not immediately obvious that myText has changed.

// However, in an immutable version of the API
let myText = NLP.text("she likes reading");

let myChangedText = myText.to_past();  // Returns a new instance without mutating myText
myText.text()  // -> she likes reading
myChangedText.text()  // -> she liked reading

// This forces the programmer to make it explicit that
// the instance has changed which helps avoids a lot of bugs.

[creatorrr]

'undo' and things?

Yeah, with an immutable API, it'd be trivial to write a plugin that manages state changes and can implement an undo/redo like feature!

If you parsed a novel, would it slow it down much?

Hmm, I don't think it would affect performance that much except it might hog more memory. If we simply do reference immutability, that is simply returning new instances for each function call (which is what I was thinking), then a misuse of the API might lead to memory leaks and creating unnecessary instances.

However, if we were to implement a structure sharing immutability, it'd actually save memory and potentially performance as well on repeat computations. But that'd be a lot more work. The good news is, on the surface both the forms would look exactly the same. So we could implement the former first and then slowly introduce structure sharing.

Example:

// Reference immutability
nlp.sentence("I am a good guy");
nlp.sentence("I am a good guy");  // Both are separate instances

nlp.sentence("I am a good guy").to_past();  // Completely new instance, occupies it's own memory

// Structure sharing immutability
nlp.sentence("I am a good guy");
nlp.sentence("I am a good guy");  // Both are internally the same instance

nlp.sentence("I am a good guy").to_past();  // -> I was a good guy
// Shares part of the tree as the previous instance except the new verb "was"

what's an immutability helper?

Oh, I actually meant an immutability wrapper. Was thinking of backwards compatibility. It could look like:

// One way is to make immutability default
// and offer a backwards compatible mutable version
// that can be deprecated over time.

let NLP = require("nlp_compromise");  // The immutable API
let mutable_NLP = require("nlp_compromise/mutable");  // The old API

// Or, the reverse

let NLP = require("nlp_compromise");  // The original API
let immutable_NLP = require("nlp_compromise/immutable");  // The new API

Let me know what you think. Happy to make PRs for any of the above ideas.

[spencermountain]

some very neat ideas, and thanks for explaining them so clearly.
I'm always happy to merge prs, and don't worry too much about breaking changes. It's always been explicit that we use semver, and move fast. ;)
i've got two questions, but feel free to start cranking away,
1) would it require a third-party library? that'd be our first, and ideally we'd be able to bake it in somehow.
2) would this throw an error?

myText = myText.to_past();

at minimum I agree it would be a more lucid and clear thinking of transformations. any thoughts @silentrob ?

i should say, in fixing another issue, i've split the sentence class into Question and Statement subclasses. The idea being that the methods hanging off sentence would be meaningfully different. I'm not terrifically clear about it, but will try to merge it in tonight.
cheers!!

[creatorrr]

1) would it require a third-party library?

Only for implementing structure sharing. We'd need persistent data structures, for which we could use immutable.js.

2) would this throw an error?

Nope. :)

[creatorrr]

I'll start hammering out PRs then!

[silentrob]

I think this discussion has been great, and the idea of having a consistent interface is key, big +1. I like the idea of having the object not mutate in place and return a new instance, and while Im a fan of immutable, I would just double check you are able to keep your client-side size goals with that implementation.

I feel at immutable'ism is like being vegetarian, it can be trendy and when nobody is looking you might have some bacon :)

[creatorrr]

Haha yeah, @silentrob has a great point there. We should def keep the library fingerprint as small as possible. We could just postpone the struct sharing until we hit a performance snag. Also, we could keep immutable as an optional peer dependency and that should avoid the bloat.

[spencermountain]

+1 i trust your judgement on these matters Diwank

[spencermountain]

one thing that weighs into this conversation is the design of the new match/replace api. If we have 10 matches from nlp.text('').match(''), and want to manipulate like half of them, it's tempting to be able to change them remotely:

t = nlp.text('the giraffes ate the apples')
results = t.match('the [Noun]')
results.doSomethingDestructive()
t.text() 
//the cool giraffes ate the cool apples

[nateabele]

Doing different things with the same set of values is actually easier if you don't do mutation. Example:

var original = nlp.text('the giraffes ate the apples')
var results = original.match('the [Noun]').doSomethingThatReturnsANewValue().text()
var newStuff = original.match(...).otherThing().text()

Going immutable also buys you a lot more flexibility in terms of performance. Depending on how much of the internals you want to expose, you could make it possible to control processing over time, distribute values across workers, etc., and if you wanted to do multiple destructive operations on a novel, that means reprocessing it if you need to do something else that conflicts with the original operations.

Hope all that makes sense.

[spencermountain]

hey, so v7 didn't go fully-immutable (though i spent a few sleepless nights this winter thinking we should)
but it did clear-up a bunch about thow this works.

i'm trying to clean-up issues, but will keep this one on-hand.

when it runs .match(), it returns a subset of terms, which is a new Text instance, but has a reference to itself - and the only times when a method needs to mutate both this and the parent reference, are when word-order is effected - insert and delete.

it's not perfect, but we'll see if v7 produces the same confusion with users as did v6. I think it's more clear.

[zenflow]

JavaScript itself has the same woes...

For instance array.filter(n => n < 3) does not mutate array, whereas array.reverse() does.

Having methods that consistently do don't cause mutation would be really nice (something I'd love to see) but otherwise it's just a matter of knowing which methods will cause mutation, and first using .slice() (or in this case .clone().

[nateabele]

@zenflow Or you could check out Ramda.

[spencermountain]

hey Matthew, yeah you know, we've got the api articulated over here, we should really just add a boolean for which methods are mutable right now. I'm re-doing the docs and will add that information in

[zenflow]

@nateabele I have checked it out, and it looks excellent. Only problem with this solution is it's not built into the language, and it's rather large to include in small modules. I should perhaps try bringing it into some other projects though...

[zenflow]

@spencermountain That is great news! Thank you for your work! I would name the boolean mutator though since mutation is something methods do, not something that happens to them.