Some minor fixes to Node port + support for device, isMobile, isSpider #56

shripadk · 2012-09-12T23:40:29Z

Even though I have read/write access to this repo, I'll continue to send pull requests so that the changes can be reviewed before merging. :) This should fix #53 and #54.

bluesmoon · 2012-09-13T05:32:55Z

js/index.js

-    obj.major = parseInt(majorVersionRep ? majorVersionRep : m[2]);
-    obj.minor = m[3] ? parseInt(m[3]) : null;
-    obj.patch = m[4] ? parseInt(m[4]) : null;
+    obj.major = ~~parseInt(majorVersionRep ? majorVersionRep : m[2]);


add a code comment stating how ~~ works with numbers and NaN

Added relevant information regarding the double bitwise not trick.

…n-digit characters

tobie · 2012-09-13T15:05:31Z

Can we please split up diffs for regexes.yaml from everything else?

Fine for merging that part. The rest of the JS changes I'd like to review separately. I'm not fond of the bitwise operator trick, unless there are proven performance benefits in what's cleary a bottleneck of the code.

Furthermore, @bluesmoon mentioned in #5 that version numbers should be presented as strings (and not ints) as we're sometimes loosing info in the process of parsing these.

I welcome a proposition of how to go about changing this to ship it in a 0.3 version.

Makes sense?

shripadk · 2012-09-13T15:31:46Z

I agree with your points except for the bitwise operator trick which is arguably more performant. But this is moot if we are not going to be parsing out integers in the first place.

But, regarding #5, there is, I think, a problem with parsing out version numbers properly. The problem lies in regexes.yaml in which most of the regular expressions use \d+ (match 1 or more digits) for version matching. So the version number is returned as a string but stripped off the other non-digit suffixes/prefixes (a,b,pre etc). We have to first make changes to all the regexes in regexes.yaml. Its nothing to do with parseInt really.

shripadk · 2012-09-13T15:44:46Z

Okay there are a few regexes in regexes.yaml which include parsing out prefixes (Firefox Namoroka/Minefield etc). So what you do propose? Is there a benefit in keeping it as string, apart from comparison between versions? If it needs to be around for comparison, should this feature be added to the module itself or should be taken care of by the user at the application level?

tobie · 2012-09-13T15:56:06Z

Not sure what's best. Would like input from @bluesmoon as he reported the issue initially.

bluesmoon · 2012-09-13T21:30:16Z

Version number comparison should be left to something like semver. I like keeping the versions as strings because that's typically what one displays in analytics software. Also, there are some user agents that only have string versions (and I forget the name of the one example that I'd reported).

tobie · 2012-09-13T21:37:27Z

semver comparison only works reliably when the version strings are conformant to then Semver spec.

What are the ports in the other languages doing? Strings or ints?

What about providing both?

shripadk · 2012-09-14T06:13:13Z

My D port uses integers for versions (don't know about other ports. haven't read the code yet). Its trivial to rewrite to use strings. I vote for having both. It would facilitate simple comparisons at least (semver can be implemented at application level). If we should have both, then we need the .major, .minor and .patch to be strings and .major.toInt, .minor.toInt, and .patch.toInt to just parse out the integer. Do you have a better API in mind?

closealert · 2012-11-07T08:06:24Z

I'm in need of the device property in node as well and noticed this PR has been open for 2 months now. @shripadk, You haven't documented the device prop in your PR but from what I can see now it only returns a string? @tobie Wouldn't it be preferable to have a similar output format for all languages?

tobie · 2012-11-07T09:12:00Z

There's too much different things going on in this PR for it to be pulled directly.

Happy to consider some more atomic changes.

shripadk · 2012-11-07T09:22:54Z

@closealert True, I missed out on documenting device proprerty. Will do so. Thanks. Also, device should be returning a string because the regexes in regexes.yaml has only a device_replacement field (no major/minor/patch). So I'm pretty sure all the other ports return only a string.

@tobie Can you tell me what to include (or not include) so I can push the changes again?

tobie · 2012-11-07T09:48:17Z

js/index.js

@@ -6,10 +6,20 @@ var file = path.join(__dirname, '..', 'regexes.yaml');
 var regexes = fs.readFileSync(file, 'utf8');
 regexes = yaml.eval(regexes);

+var mobile_agents = {};
+var mobile_user_agent_families = regexes.mobile_user_agent_families.map(function(str) {
+  mobile_agents[str] = true;


Are you not returning anything on purpose here?

Ah! I should have just used a for-loop here. Thanks for pointing this out.

closealert · 2012-11-07T09:53:23Z

@shripadk The device property is returned as Object with isMobile and isSpider inside in other languages. For example, java:

  System.out.println(c.device.family);    // => "iPhone"
  System.out.println(c.device.isMobile);  // => true
  System.out.println(c.device.isSpider);  // => false

tobie · 2012-11-07T09:54:56Z

js/index.js

+    if(!!(d(ua))) {
+      return device = d(ua);
+    }
+  });



Overall, I don't understand why you're operating these style changes.

I don't remember now as to why this was needed. Can revert back for the time being.

tobie · 2012-11-07T09:59:49Z

Commented in lots of places. Overall, would like to see this PR split up in 2-3 chunks taregting one thing only unless there's a compelling reason not to do so.

shripadk · 2012-11-07T10:16:02Z

I have replied to your comments. Most of the issues you have raised relate to whether major, minor, patch need to return strings or integers. I think if we can come to a consensus on that, I can make the required changes and push it into this PR itself. The other issues you raised will be fixed immediately. Thanks for the review :)

tobie · 2012-11-07T10:26:08Z

Sounds good. I'm wary of backwards compatibility issues, can you suggest a more elegant API which would allow us to do both?

shripadk · 2012-11-07T10:26:37Z

@closealert Thanks will be fixed :)

shripadk · 2012-11-07T10:52:16Z

@tobie For backwards compatibility we can continue returning major, minor, patch as strings. We can define a new property 'number' on the String object with a getter that would return an integer version of the string. Example:

Object.defineProperty(String.prototype, "number", {
   get: function() {
      return ~~parseInt(this);
   }
});

var u = ua.parse('Mozilla/5.0 (iPhone; CPU iPhone OS 5_1_1 like Mac OS X) AppleWebKit/534.46 (KHTML, like Gecko) Version/5.1 Mobile/9B206 Safari/7534.48.3');

console.log(u.major); // returns "5"
console.log(u.major.number) // returns 5

AFAIK, returning a integer version of major, minor, patch would be mostly useful for version comparisons and not for display. So I don't suppose any other API change would be necessary (ex: toFullString/toVersionString).

tobie · 2012-11-07T11:16:13Z

No, we can't extend the language like that in a lib. It's a Very Bad Practice(TM).

shripadk · 2012-11-07T11:25:38Z

True. I can't think of any other way. Every other way would make it verbose. The choice is either verbosity or bad practice :)

closealert · 2012-11-07T11:37:16Z

I would suggest like @tobie to split this one up:

Changes in regexes.yaml.
Improved device property.
The addition of the attributes majorInt and minorInt (or something like that) to the UserAgent object.

shripadk · 2012-11-07T12:09:09Z

@closealert I have made changes to device property and also added attributes majorInt,minorInt,patchInt. Have also documented this. Please do review it. :)

tobie · 2012-11-07T13:01:07Z

Can we please split this up. One feature per PR. Thanks.

closealert · 2012-11-07T13:06:23Z

@shripadk The changes look good but I can still see tobie's point considering the fact that there are a lot of lines in there not directly related to this fix. (for example the expansion of the UserAgent function). If you look at the original file (https://github.com/tobie/ua-parser/blob/master/js/index.js) then there are simply too many changes to merge. @tobie, close this one up and then @shripadk can create three new PR's - one for each feature.

shripadk · 2012-11-07T13:08:10Z

@tobie , @closealert sure will do ASAP :)

shripadk added 4 commits September 13, 2012 05:01

Node fixes + support for device, isMobile, isSpider

e6f4fb8

Node port: added os.major, os.minor, os.patch

e20cf3a

Updated README NodeJS example

7178674

regex for handling new rekonq ua string

21060e7

bluesmoon reviewed Sep 13, 2012
View reviewed changes

shripadk added 3 commits September 13, 2012 12:40

explained double bitwise not trick, refactored loops to use .some

2e839bb

parseInt not required anymore

2e74464

sorry. reverting back. looks like ~~ wont work when the string has no…

c064280

…n-digit characters

tobie reviewed Nov 7, 2012
View reviewed changes

shripadk added 2 commits November 7, 2012 17:25

improved device property; majorInt/minorInt/patchInt attributes added

e20fcdb

added documentation to README.markdown

b601c41

shripadk closed this Nov 7, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some minor fixes to Node port + support for device, isMobile, isSpider #56

Some minor fixes to Node port + support for device, isMobile, isSpider #56

shripadk commented Sep 12, 2012

bluesmoon Sep 13, 2012

shripadk Sep 13, 2012

tobie commented Sep 13, 2012

shripadk commented Sep 13, 2012

shripadk commented Sep 13, 2012

tobie commented Sep 13, 2012

bluesmoon commented Sep 13, 2012

tobie commented Sep 13, 2012

shripadk commented Sep 14, 2012

closealert commented Nov 7, 2012

tobie commented Nov 7, 2012

shripadk commented Nov 7, 2012

tobie Nov 7, 2012

shripadk Nov 7, 2012

closealert commented Nov 7, 2012

tobie Nov 7, 2012

shripadk Nov 7, 2012

tobie commented Nov 7, 2012

shripadk commented Nov 7, 2012

tobie commented Nov 7, 2012

shripadk commented Nov 7, 2012

shripadk commented Nov 7, 2012

tobie commented Nov 7, 2012

shripadk commented Nov 7, 2012

closealert commented Nov 7, 2012

shripadk commented Nov 7, 2012

tobie commented Nov 7, 2012

closealert commented Nov 7, 2012

shripadk commented Nov 7, 2012

Some minor fixes to Node port + support for device, isMobile, isSpider #56

Some minor fixes to Node port + support for device, isMobile, isSpider #56

Conversation

shripadk commented Sep 12, 2012

bluesmoon Sep 13, 2012

Choose a reason for hiding this comment

shripadk Sep 13, 2012

Choose a reason for hiding this comment

tobie commented Sep 13, 2012

shripadk commented Sep 13, 2012

shripadk commented Sep 13, 2012

tobie commented Sep 13, 2012

bluesmoon commented Sep 13, 2012

tobie commented Sep 13, 2012

shripadk commented Sep 14, 2012

closealert commented Nov 7, 2012

tobie commented Nov 7, 2012

shripadk commented Nov 7, 2012

tobie Nov 7, 2012

Choose a reason for hiding this comment

shripadk Nov 7, 2012

Choose a reason for hiding this comment

closealert commented Nov 7, 2012

tobie Nov 7, 2012

Choose a reason for hiding this comment

shripadk Nov 7, 2012

Choose a reason for hiding this comment

tobie commented Nov 7, 2012

shripadk commented Nov 7, 2012

tobie commented Nov 7, 2012

shripadk commented Nov 7, 2012

shripadk commented Nov 7, 2012

tobie commented Nov 7, 2012

shripadk commented Nov 7, 2012

closealert commented Nov 7, 2012

shripadk commented Nov 7, 2012

tobie commented Nov 7, 2012

closealert commented Nov 7, 2012

shripadk commented Nov 7, 2012