Compromise: Commas in whitespcae

[ohtangza]

const result = compromise.nlp(`We just released, "Square Up".`);

const textArray = result.map(obj => obj.text);
console.log(textArray);
// ['We', 'just', 'released,', 'Square', 'Up"']

I think it should be one of either case: 1) the 4th element should be'"Square', or 2) the fifth element should be Up. As far as I know, this library's text property is designed to include,, ", '`' (https://github.com/spencermountain/compromise/blob/master/src/text/methods/normalize.js#L22). So, I believe it should have been the former case.

(Btw, is it correct that spaceBefore and spaceAfter contains quotation symbols? I'm asking this because the property is named as spaceXXX.)

[spencermountain]

hi Jeungmin, yeah you're right. Something's up. I recently (in november?) changed how this works, and am not sure that was a good idea.

I've since started an over-ambitious whitespace/punctuation re-write on the dev branch, but it's gonna be a while before it's ready to roll.

I'll take a look today.

[spencermountain]

fixed in 11.12.5

[ohtangza]

@spencermountain Thanks for your response. I have one question. It seems that text prop contains comma , symbol. Is it intended behavior, or are you expecting any update on this in the future? My product heavily relies on this library, so I'm interested in the following updates.

[spencermountain]

oh, right this is a bug.

what would be the best solution for your product?
i think anything non-word, including a comma, should be in 'whitespace', or a similarly-named field.

[ohtangza]

@spencermountain I'm pretty okay for any options defined correctly. I think our devs also expect a similar direction. I think non-word info is parsed into spaceBefore and spaceAfter would be fine in the current implementation.

The only thing that could mislead devs is the word space because this word reminds devs of the term, "whitespace" like , \n, \t... But considering the backward compatibility, it should be fine for those included in spaceBefore and spaceAfter.

[spencermountain]

hey Jeungmin, this has been refactored in v12, and seems to make a little more sense:

const doc = nlp(`We just released, "Square Up".`)
console.log(doc.json({ terms: { whitespace: true } })[0])

/*{ text: 'We just released, "Square Up".',
  terms:
   [ { text: 'We', tags: [Array], pre: '', post: ' ' },
     { text: 'just', tags: [Array], pre: '', post: ' ' },
     { text: 'released', tags: [Array], pre: '', post: ', ' },
     { text: 'Square', tags: [Array], pre: '"', post: ' ' },
     { text: 'Up', tags: [Array], pre: '', post: '".' } ] }
*/

thanks for your patience.

[ohtangza]

@spencermountain Checked your message! Looks good now!

cc: @ghsdh3409