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

DeviceDetector#library? method #41

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

DeviceDetector#library? method #41

wants to merge 1 commit into from

Conversation

wallin
Copy link

@wallin wallin commented Apr 30, 2018

Expose functionality to determine whether a UserAgent is a known request library

@wallin
Copy link
Author

wallin commented May 8, 2018

@YAGoOaR any thoughts or objections?

@matisojka
Copy link
Contributor

Just to be sure on this: what would be your use case for it?

@wallin
Copy link
Author

wallin commented May 8, 2018

@YAGoOaR similar to the bot? method, but for detecting known request libraries

@matisojka
Copy link
Contributor

@wallin Thanks, but my question was more about in what context would one need that? Why would you need to detect a request library? Security? Blocking possible attackers?

@wallin
Copy link
Author

wallin commented May 9, 2018

@YAGoOaR mostly as a very crude way of detecting scripted access (for content scraping etc.). Obviously, it's easy to change the UserAgent so this would only detect the ones that don't even bother.

@wallin
Copy link
Author

wallin commented May 14, 2018

@YAGoOaR any input?

@matisojka
Copy link
Contributor

matisojka commented May 14, 2018

I feel that it's a bit niche, yet I don't see the reason why it couldn't have a place in the gem.

Now some thoughts:

I think the code should work similar to the one in the Bot class (https://github.com/podigee/device_detector/blob/develop/lib/device_detector/bot.rb)
This makes it a bit more decoupled from the client itself and more flexible / extensible in the future.

The name of the method #library? does not convince me too much either, as it sounds a bit vague. Would you call the command line versions of cURL or wget to be libraries? I know, it's defined like this in the upstream regexes, but still.

What do you think?

@wallin
Copy link
Author

wallin commented May 19, 2018

Sorry for the belated response @YAGoOaR, got caught up in other work.

I feel that it's a bit niche, yet I don't see the reason why it couldn't have a place in the gem.

Great, thanks

I think the code should work similar to the one in the Bot class

Sounds good to me. But just to confirm, this will make known? return false for these kinds of UAs, similar to how it's returning false for bot? today. Is this OK with you? I'm not entirely sure of the intention of known?

Would you call the command line versions of cURL or wget to be libraries?

I a sense yes, eg. libcurl. But sure, it's a bit contrived. Some other suggestions that come to mind: scripted?, request_tool?, tool?. Do you have a preference or other ideas?

@wallin
Copy link
Author

wallin commented May 30, 2018

@YAGoOaR any new ideas on method name? and are you ok with the changed behavior for known?

@matisojka
Copy link
Contributor

client.known? # => will return false if user_agent is unknown

So we need to stick with this definition.

I like scripted?, I think it is clear and easy to understand without even checking the docs.

@wallin
Copy link
Author

wallin commented May 30, 2018

So we need to stick with this definition.

Ok, so I assume this needs to be fixed for recognized bot UserAgents as well then? (in a separate PR of course)

I like scripted?, I think it is clear and easy to understand without even checking the docs.

Good. I'll go with that

@nijikon
Copy link

nijikon commented Jun 21, 2018

@YAGoOaR can this be merged?

@matisojka
Copy link
Contributor

@nijikon We are still waiting for changes to be done by @wallin

@wallin I think that #known? should also return true even for bots, but if that's not the case, we should not change the behavior by now, since we might break compatibility. I guess this would be an improvement for 2.0, but even then I would create a new method with a different name to avoid any possible confusion.

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.

3 participants