New IRC message parser #972

xPaw · 2017-03-18T08:52:31Z

Supersedes #699. Brings the parser code back into the repository, does not use a polyfill, fixes node4 compat (for tests) @Bonuspunkt gave his OK in #699 (comment)

Fixes #654, #15, #199, #583, #928, #1001.

Let's get this merged and work on issues (if any) along the way, it's been a PR for a while now, we do really need to get this moving.

williamboman · 2017-03-29T19:57:37Z

Just a nitpick, but how much of this was authored by @Bonuspunkt? I think it'd be nice to keep authorship (in terms of commits) where it's due.

xPaw · 2017-03-31T05:44:34Z

@astorije rebase and CI builds should be green. I've been running this PR on my instance for a couple of weeks now, i am yet to see any problems.

xPaw · 2017-03-31T05:45:04Z

@williamboman We definitely can do that before the merge

astorije · 2017-03-31T05:47:38Z

@astorije rebase and CI builds should be green.

A few conflicts so I'll have to do that with a fresh head, not at 2am :)

dgw · 2017-03-31T05:49:20Z

A few conflicts so I'll have to do that with a fresh head, not at 2am :)

You mean you don't dream in git commands? 😮

xPaw · 2017-03-31T16:07:02Z

Rebased and changed commit authorship to @Bonuspunkt.

kode54 · 2017-04-03T01:18:31Z

This parser currently mangles the topic to ##windows:

MS Windows Support & Discussion. Keep it polite, factual, constructive, and topical. || Site and Guidelines: http://goo.gl/7ueKMi || Server ##windows-server || Games try ##windows-gaming || Off topic /msg alis list SEARCHTERM

Which gets mangled into:

MS Windows Support & Discussion. Keep it polite, factual, constructive, and topical. || Site and Guidelines: http://goo.gl/7ueKMi || Server http://goo.gl/7ueKMi || Server ##windows-server || Games try ##windows-gaming || Off topic /msg alis list SEARCHTERM##windows-server || Games try ##windows-gaming

Pasting that mess into a channel results in the following further mangling:

MS Windows Support & Discussion. Keep it polite, factual, constructive, and topical. || Site and Guidelines: http://goo.gl/7ueKMi || Server http://goo.gl/7ueKMi || Server http://goo.gl/7ueKMi || Server http://goo.gl/7ueKMi || Server ##windows-server || Games try ##windows-gaming || Off topic /msg alis list SEARCHTERM##windows-server || Games try ##windows-gaming##windows-server || Games try ##windows-gaming || Off topic /msg alis list SEARCHTERM##windows-server || Games try ##windows-gaming

astorije

It's really cool that this PR comes with so many tests, but it desperately needs comments.
I have tried to review it but I really can't follow the logic of it, which also means I have no way to identify issues or debug if necessary.

@xPaw, I know you are not near a computer during the week, but if you happen to post comments on lines of this PR, I can integrate them as comments in the file. Thanks!

astorije · 2017-04-03T01:15:06Z

client/js/libs/handlebars/ircmessageparser/merge.js

+
+	return allParts.map(textPart => {
+		textPart.fragments = styleFragments
+            .filter(fragment => anyIntersection(textPart, fragment))


What is the purpose of anyIntersection here? Not sure I'm following.

@Bonuspunkt, thanks a lot for your previous comments. That helped me commenting the code and understanding/reviewing the flow/edge cases of findChannels.
Would you be able to help me on the rest of this PR? Ideally, add a comment to lines that may seem confusing to someone who hasn't written this code to explain why this was done or necessary.

Thanks!!

you got the concept of the parser?
you get a message like \x02visit irc\x0f://\x1dfreenode.net\x0f/\x034,8thelounge

step is extract the styling information and get the plain text

on the plain text, find channels and urls, fill rest with simple "text" object

merge the styling information with the channels / url / text objects
and in the case of lounge

generate html strings with result of step 3

astorije · 2017-04-03T03:51:08Z

client/js/libs/handlebars/ircmessageparser/findChannels.js

+const escapeRegExp = require("lodash/escapeRegExp");
+
+// NOTE: channel prefixes should be RPL_ISUPPORT.CHANTYPES
+// NOTE: userModes should be RPL_ISUPPORT.PREFIX


Are these TODOs for later, or addressed and comments were left there?

duplicates - see parser.js

Ok, will remove these.

astorije · 2017-04-03T03:52:27Z

client/js/libs/handlebars/ircmessageparser/findChannels.js

+	const channelPrefixPattern = channelPrefixes.map(escapeRegExp).join("");
+
+	const channelPattern = `(?:^|\\s)[${ userModePattern }]*([${ channelPrefixPattern }][^ \u0007]+)`;
+	const channelRegExp = new RegExp(channelPattern, "g");


What does this RegExp mean/do?

userpattern is for finding in whois response

Really cool, thanks!

astorije · 2017-04-03T03:53:44Z

client/js/libs/handlebars/ircmessageparser/merge.js

+});
+		return textPart;
+	});
+}


Most likely an indentation issue in this file. Also fairly cryptic function.

astorije · 2017-04-03T03:54:25Z

client/js/libs/handlebars/parse.js

+		if (text.indexOf(url, lastPosition) < 0) {
+			return;
+		}
+		// ^-- /fix: url was modified and does not match input string -> cant be mapped


What does fix mean here? FIXME?

did fix an issue with uri detection

Was it an issue with the URI lib? Is it still broken?
I remember you had a few issues/PRs open there.

no, not for any of the known testcases

Sorry I meant is it still broken in URI.js? Meaning, is this still needed pending a fix in URI.js or is it now unnecessary to keep in our own codebase?

astorije · 2017-04-03T03:59:03Z

Thanks for your comment @kode54! I have tried to review the PR and got lost within the 1000 extra lines so I'll wait for @xPaw and/or @Bonuspunkt to help with commenting it and making it easier to follow before I can hope to help on that.

astorije · 2017-04-04T04:43:28Z

@kode54, I'm reviewing this one bit at a time so when I have a good grasp of it (i.e. when I get the chance to spend more than a few minutes on it), I'm sure I'll have more understanding to help with this.

At the moment, I have reviewed findChannels which seems ok to me, and using it to parse your message, I get the following (correct) output:

> findChannels("MS Windows Support & Discussion. Keep it polite, factual, constructive, and topical. || Site and Guidelines: http://goo.gl/7ueKMi || Server ##windows-server || Games try ##windows-gaming || Off topic /msg alis list SEARCHTERM", ["#", "&"], ["!", "@", "%", "+"])
[ { start: 140, end: 156, channel: '##windows-server' },
  { start: 170, end: 186, channel: '##windows-gaming' } ]

So that's not it, maybe next time 😅

astorije · 2017-04-04T04:47:37Z

xPaw · 2017-04-04T17:03:29Z

@astorije, sounds to me like link parser is fucking it up.

Bonuspunkt · 2017-04-04T20:57:37Z

@xPaw @astorije Bonuspunkt/ircmessageparser@5a249c3 fixes it

astorije · 2017-04-17T06:08:16Z

client/js/libs/handlebars/ircmessageparser/parseStyle.js

-			fragment.start = i === 0 ? 0 : array[i - 1].end;
-			fragment.end = fragment.start + fragment.text.length;
-			return fragment;
-		});


I have made this .map unnecessary by adding start/end information as part of the emitFragment() function (see above) and summing up end in the optimization step above.

astorije · 2017-04-18T04:20:36Z

client/js/libs/handlebars/ircmessageparser/findLinks.js

+		if (text.indexOf(url, lastPosition) < 0) {
+			return;
+		}
+		// ^-- /fix: url was modified and does not match input string -> cant be mapped


@Bonuspunkt, @xPaw, I'm still confused: is this fix still relevant?
There is no test case for it (i.e. coverage does not go inside that if and commenting it out does not make the test suite to fail). If it's still relevant, what would be a good example to add to the test suite? If it's not relevant, then we should remove it.

no, but i would leave it because if there is a problem it goes horrible wrong
(you can verify with medialize/URI.js#325 )

If the fix is in the upstream project, then we shouldn't bother with keeping this. Also, they added a test case for it, so hopefully there won't be a regression.
Thanks for your help!

xPaw · 2017-04-18T08:39:57Z

client/js/libs/handlebars/ircmessageparser/merge.js

@@ -0,0 +1,34 @@
+"use strict";
+
+const _ = require("lodash");


You're now including entire lodash in our client.

I thought we were using it there already but I was wrong, my bad. I'll remove, thanks for catching this!

It turns out that the polyfill is not necessary for our test suite to pass and browsers we currently support should be fine with Object.assign used in cc7d121.
Am I missing something?

IE11 - i'm pretty sure if you don't support it, you don't have to transform ES6 to ES5

Right, but I was pretty sure we dropped IE support a long time ago and we expected Windows users to run Edge. @xPaw, what do you think?
If not, then we should use https://babeljs.io/docs/plugins/transform-object-assign/ instead, no?

Let's keep the polyfill for now. I believe IE10 should be still supported. I will take a look at browser support some time later.

Fair enough, removed.

astorije · 2017-04-19T05:01:54Z

Just one question left regarding that findLinks fix (see #972 (comment)) and then I should be all set.

I spent a lot of time reviewing all that, making sure I was following everything correctly so I could comment things in a useful manner, really great job @Bonuspunkt!

astorije · 2017-04-21T04:23:18Z

Status for self: awaiting answer from @xPaw about #972 (comment) then me addressing, then deal shall be sealed.

astorije

This is some damn sweet job right there! Sorry it took me so long to review, but I wanted to make sure I could follow and document everything before we can proceed.

It's kind of hard to follow at first, the logic is quite specific, but it makes sense after a while (Stockholm syndrome 😅) and the insane test coverage helps a lot in understanding what piece should do what. In fact everything but the Object.assign polyfill (lol!) gets covered at least once here!

I have rebased this PR and squashed my many commits into logical chunks so that this can be merged as-is and keep a meaningful history.

Thank you so very much for your efforts, @Bonuspunkt, greatly appreciated!
Would you mind taking a last look and giving your approval for this before I merge?

xPaw · 2017-04-21T06:01:05Z

🎉

Bonuspunkt

lgtm

Bonuspunkt · 2017-04-21T10:32:03Z

client/js/libs/handlebars/ircmessageparser/fill.js

@@ -0,0 +1,34 @@
+"use strict";
+
+// Create dummy entries corresponding to areas of the text that match no


"dummy entries" -> "plain text entries"

Bonuspunkt · 2017-04-21T13:48:00Z

client/js/libs/handlebars/ircmessageparser/merge.js

+
+let Object_assign = Object.assign;
+
+if (typeof Object.assign !== "function") {


would monkeypatch or check Object_assign

What do you mean? This is the same check than in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Polyfill. I thought you added it but maybe @xPaw did. I didn't touch that part (well I did then I removed lol).

if (typeof Object_assign !== "function")

or ditch the Object_assign and just assign to Object.assign
iirc the Object_assign was not the polyfill and just an incorrect implementation that was "good enough"

Fixes #15. Fixes #199. Fixes #583. Fixes #654. Fixes #928. Fixes #1001.

Tests were taken from https://github.com/Bonuspunkt/ircmessageparser/blob/5a249c30b1e0379e5df62f10734a3dc7ce02306b/test/findLinks.js. The underlying code is different but the tests are the same.

Also make it output double quotes for consistency with web stuff.

- Add comments and descriptions to: - `findChannels.js` - `parseStyle` - `findLinks` - `fill` - `anyIntersection` - `merge` - `parse` - Minor optimizations to `parseStyle` - Add tests for `fill`

See medialize/URI.js#325

astorije · 2017-04-21T17:41:31Z

Wooooh huge thanks to @Bonuspunkt and @xPaw and others!!

…rser New IRC message parser

xPaw added Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. Type: Feature Tickets that describe a desired feature or PRs that add them to the project. labels Mar 18, 2017

xPaw added this to the 2.3.0 milestone Mar 18, 2017

xPaw changed the title ~~Bonuspunkt/message parser~~ New IRC message parser Mar 18, 2017

xPaw force-pushed the bonuspunkt/message-parser branch from 52836e1 to fffdf40 Compare March 18, 2017 09:09

This was referenced Mar 18, 2017

Use require() instead of import in client code #973

Merged

improve client message parsing #699

Closed

xPaw assigned astorije Mar 18, 2017

xPaw force-pushed the bonuspunkt/message-parser branch from fffdf40 to a1476ff Compare March 31, 2017 16:06

xPaw force-pushed the bonuspunkt/message-parser branch from a1476ff to 7fd91b4 Compare March 31, 2017 16:12

astorije mentioned this pull request Apr 1, 2017

URL parser thinks > is a valid URL character #1001

Closed

astorije requested changes Apr 3, 2017

View reviewed changes

astorije assigned xPaw and unassigned astorije Apr 3, 2017

astorije mentioned this pull request Apr 3, 2017

&& (and most likely others like that) should not be parsed as channel #443

Closed

Bonuspunkt mentioned this pull request Apr 3, 2017

FIX: parts where not sorted resulting in incorrect output Bonuspunkt/ircmessageparser#5

Merged

astorije force-pushed the bonuspunkt/message-parser branch from 3847fc4 to 5371978 Compare April 4, 2017 04:40

astorije force-pushed the bonuspunkt/message-parser branch from 5371978 to 35cfedb Compare April 4, 2017 05:36

astorije reviewed Apr 17, 2017

View reviewed changes

astorije reviewed Apr 18, 2017

View reviewed changes

xPaw commented Apr 18, 2017

View reviewed changes

astorije force-pushed the bonuspunkt/message-parser branch from b81da59 to 92f0910 Compare April 19, 2017 03:42

astorije force-pushed the bonuspunkt/message-parser branch from bb8c8b5 to b894c61 Compare April 20, 2017 05:41

astorije mentioned this pull request Apr 21, 2017

Change index.html to be rendered using handlebars #1057

Merged

astorije unassigned xPaw Apr 21, 2017

astorije force-pushed the bonuspunkt/message-parser branch from b894c61 to a2cb3a5 Compare April 21, 2017 05:30

astorije approved these changes Apr 21, 2017

View reviewed changes

Bonuspunkt approved these changes Apr 21, 2017

View reviewed changes

xPaw force-pushed the bonuspunkt/message-parser branch from a2cb3a5 to 937a77b Compare April 21, 2017 17:26

Bonuspunkt and others added 6 commits April 21, 2017 20:29

Add bonuspunkt's parser

0b85ded

Fixes #15. Fixes #199. Fixes #583. Fixes #654. Fixes #928. Fixes #1001.

Add message parser tests

eb1360c

Extract findLinks into its own file and add tests

5b4c00d

Tests were taken from https://github.com/Bonuspunkt/ircmessageparser/blob/5a249c30b1e0379e5df62f10734a3dc7ce02306b/test/findLinks.js. The underlying code is different but the tests are the same.

Use template literals in parse

90f4a94

Also make it output double quotes for consistency with web stuff.

Explain the modules of the message parser and add tests

03e3444

- Add comments and descriptions to: - `findChannels.js` - `parseStyle` - `findLinks` - `fill` - `anyIntersection` - `merge` - `parse` - Minor optimizations to `parseStyle` - Add tests for `fill`

Remove URI.js monkey-patch as fix landed in v1.18.5

fa1aecd

See medialize/URI.js#325

xPaw force-pushed the bonuspunkt/message-parser branch from 937a77b to fa1aecd Compare April 21, 2017 17:31

astorije merged commit 6c0cf7c into master Apr 21, 2017

astorije deleted the bonuspunkt/message-parser branch April 21, 2017 17:40

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#972 from thelounge/bonuspunkt/message-pa…

d4975c8

…rser New IRC message parser

		@@ -0,0 +1,34 @@
		"use strict";

		// Create dummy entries corresponding to areas of the text that match no


		let Object_assign = Object.assign;

		if (typeof Object.assign !== "function") {

New IRC message parser #972

New IRC message parser #972

Conversation

xPaw commented Mar 18, 2017 • edited Loading

williamboman commented Mar 29, 2017

xPaw commented Mar 31, 2017

xPaw commented Mar 31, 2017

astorije commented Mar 31, 2017

dgw commented Mar 31, 2017

xPaw commented Mar 31, 2017

kode54 commented Apr 3, 2017 • edited Loading

astorije left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bonuspunkt Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Apr 3, 2017

astorije commented Apr 4, 2017

astorije commented Apr 4, 2017 • edited Loading

Status of my review

xPaw commented Apr 4, 2017

Bonuspunkt commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Apr 19, 2017

astorije commented Apr 21, 2017

astorije left a comment • edited Loading

Choose a reason for hiding this comment

xPaw commented Apr 21, 2017

Bonuspunkt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bonuspunkt Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

astorije commented Apr 21, 2017

xPaw commented Mar 18, 2017 •

edited

Loading

kode54 commented Apr 3, 2017 •

edited

Loading

Bonuspunkt Apr 4, 2017 •

edited

Loading

astorije commented Apr 4, 2017 •

edited

Loading

astorije Apr 19, 2017 •

edited

Loading

astorije left a comment •

edited

Loading

Bonuspunkt Apr 21, 2017 •

edited

Loading