-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Listens for links, chat complete a video if that's where the link points #91
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
import Listener from './Listener' | ||
|
||
/** | ||
* Listens for URLS entered by the user. Matches to end of string only. | ||
* | ||
* @inheritdoc | ||
*/ | ||
|
||
const endingURLRegexp = /((http[s]?|ftp):\/)?\/?([^:\/\s]+)((\/\w+)*\/)([\w\-\.]+[^#?\s]+)(.*)?(#[\w\-]+)?$/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry to be a drag, but we've gotta add some basic tests for this new code. Code like this (and especially hairy regexes) are basically impossible to touch in the future without some tests in place. If we get lax right off the bat, I'm worried other.js will immediately become legacy code rather than a solid foundation to build upon. The existing tests should be pretty straightforward to copy/paste/hack. I'm happy to help. See: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense that we want to start our other.js code culture right. Here's a philosophical question: I wrote this is an experience test—how does something like this feel to gain intuition on embedded media: it was dL with minimal dW. For experimental commands, it feels like we want to minimize the dW for us all to play, while still retaining and encouraging good coding practices if we likes it. How does we do? One solution: Maybe I would have committed this to an experimental pack of commands that works on beta.other.chat and in the enterprise app. When it's time to move it over to builtins, then it has to be fully vetted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, agreed. The way things are set up now is that there's no code coverage or similar requirements on the experimental directory. My comments about this only refer to builtins (which is what this patch is modifying). |
||
|
||
class LinkListener extends Listener { | ||
/** | ||
* @callback LinkListener#onCallback | ||
* @return {?(Promise|Listener~Result)} - The resulting action that the | ||
* user agent should take in response to this ;oml. | ||
*/ | ||
|
||
/** | ||
* @param {LinkListener#onCallback} on - Called when one a link is found. | ||
*/ | ||
constructor({on}) { | ||
super() | ||
this._on = on | ||
} | ||
|
||
onActivateChatCompleteResult(action, result, message) { | ||
// TODO: This does not seem to get called? | ||
// That is, adding a console.log('boom') will not print 'boom' | ||
// and adding a return doesn't effect behavior. | ||
if (action !== 'default' || !result.text) return null | ||
return {stagedMessage: {text: result.text}} | ||
} | ||
|
||
onSetStagedMessage(message) { | ||
// Get text typed so far | ||
const {text} = message | ||
|
||
const match = text.match(endingURLRegexp) | ||
const linkURL = match ? match[0] : null | ||
|
||
if (linkURL && !linkURL.match(/ /)) { | ||
const result = this._on({url: linkURL}) | ||
return result instanceof Promise ? result : Promise.resolve(result) | ||
} | ||
|
||
return Promise.resolve({chatCompletions: []}) | ||
} | ||
} | ||
|
||
export default LinkListener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice!
Travis is sad about the code formatting: https://travis-ci.com/other-xyz/other.js/jobs/54105617
Would you mind please fixing it up first? I'm happy to consider adjusting any of our style rules (loved the semicolon nixing), however I've seen a lot of benefit from adhering to some set of guidelines consistently.
The easiest workflow for sticking to the style guide is to install an eslint plugin for your editor of choice. Instructions for Atom and Sublime are here:
https://github.com/other-xyz/other.js/blob/master/CONTRIBUTING.md#initial-setup
Or at the end of the day, just make sure the tests are happy:
https://github.com/other-xyz/other.js/blob/master/CONTRIBUTING.md#testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
Visually speaking, I enjoy JS coding styles that have more space around things. I find it easier to follow/read. I have also found (N=1) that visually tighter code incents me to write more clever/terse code so as to fit in.
I guess I have preferences and also feel like my preferences don't count too strongly because I am not in the code everyday. Good PSP stuffs.