search

ktaragorn / mobile_detect

Ruby port of php library of the same name
MIT License
7 stars 2 forks source link

Update rules to 2.8.19 #1

Closed wallin closed 8 years ago

wallin commented 8 years ago

Other changes

ktaragorn commented 8 years ago

Thanks a lot for these changes/fixes, I am still processing it. If these are in seperate branches in your fork can you submit them as seperate PRs? Else nvm.

Do you often use ruby < 1.9.3? The only change I'm not a fan of is the hash rocket one.

ktaragorn commented 8 years ago

The scripts you should rename to be more explicit, perhaps drop the setup script and rename console to mobile_detect or similar. These scripts in bin/ would be added to the PATH unless I am much mistaken, and console is a very generic name, and not much related to mobile_detect.

ktaragorn commented 8 years ago

After merge (for my own reference):

wallin commented 8 years ago

Hi, thanks! Yeah, I should have made separate branches/PRs, sorry for this. Just that I needed this in my own app and wasn't sure if I could use other branches in the Gemfile

  1. I'll be happy to skip Ruby 1.9.3 as it was deprecated over a year ago.
  2. The scripts (including the names of them) are the same as generated when initializing a new gem with bundle gem my_gem so this is pretty standard. But I have no strong opinion so feel free to rename them to whatever you want.
ktaragorn commented 8 years ago

Hi,

I have been in that situation, no problem. For future reference you can indeed use branches in gemfile, when you specify a git: or a github: gem, you can pass in a branch: option. The way I would have done it, would be to make the changes into seperate branches, merge them all into master (your master) and use that master in your app. Then you can submit PRs from those individual branches, and still use all the changes at once in your app.

Ruby 1.9.3 can use the colon syntax for hashes, it is 1.9.2 that cannot, and that is a far older version.

Do you find yourself using the console very often? i.e is it worth having?

Thanks

On 10 March 2016 at 01:30, Sebastian Wallin notifications@github.com wrote:

Hi, thanks! Yeah, I should have made separate branches/PRs, sorry for this. Just that I needed this in my own app and wasn't sure if I could use other branches in the Gemfile

  1. I'll be happy to skip Ruby 1.9.3 as it was deprecated over a year ago.
  2. The scripts (including the names of them) are the same as generated when initializing a new gem with bundle gem my_gem so this is pretty standard. But I have no strong opinion so feel free to rename them to whatever you want.

— Reply to this email directly or view it on GitHub https://github.com/ktaragorn/mobile_detect/pull/1#issuecomment-194414393 .

wallin commented 8 years ago

Thanks, that makes sense. I can do that, to make these updates easier to manage.

Re. 1.9.3 and hash-rocket, the only reason I changed it was because of the failing tests in https://circleci.com/gh/ktaragorn/mobile_detect/3. However I noticed now that they're running it on 1.9.1! So I'd skip that.

Re. console. Personally I use it all the time during development and debugging, so I'd keep it :)

ktaragorn commented 8 years ago

@wallin thank you for your support, I have released a version 0.2.0 with these changes