blakmatrix / node-migrator-bot

Migrate your old Node.js Repos
MIT License
31 stars 3 forks source link

Hello! here's my contribution #15

Closed KirillTemnov closed 12 years ago

KirillTemnov commented 12 years ago

I found out, that your replacement change variable name from sys to util, It's very dangerous, not because you can't replace all sys to util, but because I can have another util variable, that overlap new one, or vice versa,

check this file: https://github.com/node-migrator-bot/oauth-server/blob/6ab7bc852408e400f7158318688bf50a07195f24/lib/server.js

so, I've fixed that and add CoffeeScript support.

blakmatrix commented 12 years ago

Very good catch! I had considered name conflicts at one point but I guess it slipped my mind later when I was coding. My plan was to just default to only replace the require part if another util was found. That being said, I'd like to verify the new regex today or tommorow.

I will be rearranging the code in the next week to expose the parts that determine what file types to look at, what to do, and the commit/PR messages. People then can use the bot for however complex they want to get and for potentially other cases outside of node.js.

THAT being said since I have already run the JS cases it would still be usefull to run the test on NPm that use coffee files only

blakmatrix commented 12 years ago

Kirill Temnov,

I have since changed the layout of the code, and while it is still worth it to look for coffescript files(and only coffeescript since i have since ran the bot on JS) that meet the criteria, however I have moved on, if you take a look at the code now its alot more seperated out, I still have some more plans but not much should change... users can for add in thier function, set thier settings to go how far they want... One idea I have is to allow for an array of changes/commits to be made before the final pull request... I will also try and extract out parts into an examples folder. sorry for not merging, but things moved quickly and focus changed