This repository has been archived by the owner. It is now read-only.

Merging the code between stocks.coffee and stock.coffee and removing stocks.coffee #1334

Merged
merged 1 commit into from Mar 10, 2014

Conversation

Projects
None yet
3 participants
@johnwyles
Contributor

johnwyles commented Feb 18, 2014

No description provided.

@technicalpickles

View changes

src/scripts/stock.coffee
@@ -14,10 +14,11 @@
# eliperkins
module.exports = (robot) ->
- robot.respond /stock (info|price|quote) for @?([\w .-_]+)/i, (msg) ->
+ robot.respond /stock (info|price|quote)? for @?([A-Za-z0-9.-_]+)\s?(\d+\w+)?/i, (msg) ->

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles Feb 19, 2014

Member

To make it more compatible with stocks.coffee, maybe add me to the optional things, ie (info|price|quote|me)?. Similarly, make foroptional.

Another tip here... you can make these groups non-capturing with ?: in the beginning of the parens. This makes it nice to have less msg.matches to track indexes of.

@technicalpickles

technicalpickles Feb 19, 2014

Member

To make it more compatible with stocks.coffee, maybe add me to the optional things, ie (info|price|quote|me)?. Similarly, make foroptional.

Another tip here... you can make these groups non-capturing with ?: in the beginning of the parens. This makes it nice to have less msg.matches to track indexes of.

@johnwyles

This comment has been minimized.

Show comment Hide comment
@johnwyles

johnwyles Feb 20, 2014

Contributor

Done! This should work now

Contributor

johnwyles commented Feb 20, 2014

Done! This should work now

@technicalpickles

This comment has been minimized.

Show comment Hide comment
@technicalpickles

technicalpickles Feb 21, 2014

Member

Thanks!

My last concern is that existing users have stocks.coffee in their hubot-scripts.json. Maybe you could back stocks.coffee with documentation that it's been moved, and then:

modules.exports = (robot) ->
  robot.logger.warning "stocks.coffee has been merged with stock.coffee, update hubot-scripts.json to use stock.coffee instead"
  require('./stocks.coffee')(robot)
Member

technicalpickles commented Feb 21, 2014

Thanks!

My last concern is that existing users have stocks.coffee in their hubot-scripts.json. Maybe you could back stocks.coffee with documentation that it's been moved, and then:

modules.exports = (robot) ->
  robot.logger.warning "stocks.coffee has been merged with stock.coffee, update hubot-scripts.json to use stock.coffee instead"
  require('./stocks.coffee')(robot)
@johnwyles

This comment has been minimized.

Show comment Hide comment
@johnwyles

johnwyles Feb 24, 2014

Contributor

OK, fixed up the changes you recommended.

Contributor

johnwyles commented Feb 24, 2014

OK, fixed up the changes you recommended.

technicalpickles added a commit that referenced this pull request Mar 10, 2014

Merge pull request #1334 from johnwyles/stocks.coffee-and-stock.coffe…
…e-merge

Merging the code between stocks.coffee and stock.coffee and removing stocks.coffee

@technicalpickles technicalpickles merged commit c093254 into github:master Mar 10, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.