gshutler / useragent

HTTP User Agent parser
MIT License
179 stars 159 forks source link

Refactor Operating System implementation #6

Closed EdgarOrtegaRamirez closed 8 years ago

EdgarOrtegaRamirez commented 9 years ago

This will allow new operating systems to be added on a easy way.

gshutler commented 9 years ago

I find this much harder to follow in general. Previously, all the code fit in 45 lines of a single file which was quite easy to understand (and I'm not the original maintainer). With this change, in order to understand how iOS versions are extracted I need to have 3 files open.

Without there being a new operating system to add I can't see how this is better overall than what is there already, though I'm willing to be convinced otherwise.

I will say I do prefer your usage of RegExp#match rather than relying on $ variables. If you were to open a PR with just that I would merge that in a heartbeat!

ziggythehamster commented 9 years ago

@gshutler if this gem is going to be the end all be all of user agent processing, this PR makes sense to me, as it allows us to add many more operating systems (i.e., Linux, Android, watchOS, BlackBerry, ...) without making giant unmaintainable files.

I'd like to see the OS class made available to other parts of the system though. In a user agent parser, it'd be nice to be able to go operating_system.is_a?(MacOsX) or something like that.

gshutler commented 9 years ago

If this were adding more operating systems it would make a more compelling case for the refactor. As it doesn't I'm erring on the side of YAGNI as it is less clear, to me, after the change than before it.

If it surfaced the OS class, again that's something that would make it a more compelling change (but would need some consideration as to how to avoid it being a breaking change given the current functionality) as that would potentially eliminate confusion such as #5, but it doesn't do that either.

A refactor done in the context of such changes may end up being done differently as they would be guided by a use case. That is why I'm reluctant to merge this PR as it stands.

sandstrom commented 9 years ago

+1 Being able to parse Linux and others would be very useful.

kuraga commented 8 years ago

+1 to solve this, Edge and Linux!