Listens for links, chat complete a video if that's where the link points #91
Conversation
| @@ -71,6 +71,51 @@ feature.listen({ | |||
| } | |||
| }) | |||
|
|
|||
| // Media Picker | |||
There was a problem hiding this comment.
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.
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.
| * @inheritdoc | ||
| */ | ||
|
|
||
| const endingURLRegexp = /((http[s]?|ftp):\/)?\/?([^:\/\s]+)((\/\w+)*\/)([\w\-\.]+[^#?\s]+)(.*)?(#[\w\-]+)?$/ |
There was a problem hiding this comment.
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:
https://github.com/other-xyz/other.js/blob/master/builtins/__tests__/core.spec.js
There was a problem hiding this comment.
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.
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).
|
@tonygentilcore Updated style and added some tests. |
Please see comments for why YouTube .mp4’s don’t work.
Instagram and Facebook are working thought!