Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support bots and phones #52

Merged
merged 8 commits into from May 8, 2014
Merged

Support bots and phones #52

merged 8 commits into from May 8, 2014

Conversation

pepijndevos
Copy link
Contributor

I spent a good deal of time adding bots and phones until even myself could not tell what that agent string meant. What is left is a pile of unintelligible mess.

With that said, this patch supports every bot and feature phone that provided some useful info and occured more than a few times in a day worth of logs on a busy website.

One possible improvement would bo to assign types to browsers, so I can easily discern the bots from the phones from the desktops.

schwers and others added 4 commits January 21, 2014 17:24
Even the feature phones with WAP...
This can be desktop, mobile or bot.
There is some ambiguity with phablets, tablets, netbooks, and game consoles obviously.
There should be no such confusion about bots...
@pepijndevos
Copy link
Contributor Author

Ok, I added a kind, which makes it possible for users to broadly categorize traffic without worrying whatever MAUI or PhantomJS is.

This is really important for filtering robots from your statistics.

Mobile is less essential and the meaning of mobile is a bit blury these days. It could be used to display the mobile version of a website, but that would be slightly wrong.

@pepijndevos pepijndevos mentioned this pull request May 7, 2014
@shon
Copy link
Owner

shon commented May 8, 2014

Thanks much @pepijndevos Bot detection is major enhancement :)

tests are failing

python setup.py test
# or python tests.py

I am sure above are minor issues. However it would be nice to have few more tests for bots mobiles especially popular ones like Googlebot.

@shon
Copy link
Owner

shon commented May 8, 2014

I like the introduction of kind. However except kind:bot it could be somewhat unreliable.

Here are my concerns

What do you think?

@pepijndevos
Copy link
Contributor Author

I'll have a look at the tests. I have a big json file with agent strings from the logs, I can extract some test data from that, or maybe the whole file could be used somehow?

I agree with your concerns about mobile detection. I think that it could be improved somewhat, but it'll never be 100%, so it should serve as an indication only.

If I merge the Android pull, detect Windows Phones and weed out iPads, I think that would be good enough for statistic purposes. For serving different content, there will always be edge cases.

I looked at your second link, and it's one huge regex. Not the way to go.

Another issue I'd like your opinion about. Right now some bots are split up for the image, mobile, etc. versions. Maybe it'd be better to merge these to a single bot? Not so sure about the google feed fetcher. It's from google, but actually a different thing.

@pepijndevos
Copy link
Contributor Author

What's the deal with Safari on Linux, Android and other things that are not Mac or Windows? Shouldn't that be some other Webkit derivative?

iPad is actually detected as a desktop because neither Safari of iPad
are marked mobile, but iPhone is.
there is no real version number.
@shon
Copy link
Owner

shon commented May 8, 2014

@pepijndevos thanks again for all the work. I would merge you patch however I will revert "kind" support. It is not reliable and would set incorrect expectations. For bots I will add a new attribute object.bot = T/F.

I didn't understand the Webkit derivative question.

shon added a commit that referenced this pull request May 8, 2014
@shon shon merged commit 690ffbb into shon:master May 8, 2014
@pepijndevos
Copy link
Contributor Author

Oh... I added some more tests. I think I'm done now.

I understand your view on mobile detection...

There is no Safari for Linux or Android, so these are other browsers based on Webkit.

@pepijndevos
Copy link
Contributor Author

Actually Windows Phone is not yet added either.

@shon
Copy link
Owner

shon commented May 8, 2014

Are you planning to add Windows Phone?

@pepijndevos
Copy link
Contributor Author

Working on it... should I make a new pull, or will you just pull it from my fork anyway?

@shon
Copy link
Owner

shon commented May 8, 2014

I can pull from you fork.

@pepijndevos
Copy link
Contributor Author

Ok done.

With this change, I'd say mobile detection is quite reliable.
But if you don't want it, that's fine.

@shon
Copy link
Owner

shon commented May 8, 2014

With Mobile detection issue is Tablets detected as mobile. I am not convinced with that.

I have pushed my changes. I will pull your windows phone changes and then will release new version late today.

@pepijndevos
Copy link
Contributor Author

Cool.

Well, it properly detects iPads, Windows tablets, PlayBooks.

Android is more tricky. In general the thing to look for is Mobile Safari, but this is not a 100% guarantee. But if we add Mobile Safari as a Mobile browser and make Android not mobile, that would catch all phones and very few Android tablets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants