Remove class methods (without the pattern loader)

[toolmantim]

This is another take on removing the class method, but without the pattern loader refactor from #4.

Did you need the pattern loader abstraction in #4 for anything? If not, we should just start with this.

The doc fixes and test refactors are good though.

[toolmantim]

@jnunemaker thoughts?

[jnunemaker]

The nice part of the pattern loader is that you don't have to do this. Technically this makes it so that there is no cleanup before/after each test. It was a horrible hack I did previously to speed things up until I could add the pattern loader.

[jnunemaker]

I was at the github summit so I was way behind on stuff like this. Sorry.

Personally, this suffers from the same problems as the class level stuff. The main issue I see is that the parser is responsible for reading and loading the regexes, but I do not feel it should be.

Any specific reactions to the pattern loader?

[jnunemaker]

For example, what if I wanted to load patterns from the database so I didn't have to redeploy for each regex check? That would cause pain for this version that forces the file path.

[toolmantim]

@jnunemaker no worries! Have you got an example database adapter? Which hooks/methods do you need? The ua-parser patterns are 6.4MB. You wouldn't want to be doing this per-request obviously. Another way to solve the redeploy problem is to spin the ua-parsing into it's own separate sinatra app service, one that just loads the YAML off disk as normal but can be updated & deployed separately from the main app.

[jnunemaker]

I'm not actually loading from the database. That was just an example of another way the data might need to be loaded. I think the key to the pattern loader is just that it separates the concerns of parsing user agents and loading patterns.

[toolmantim]

Another way to cut it would be to move out all browserscope-specific pattern loading and matching, and have pluggable/chainable matcher backends. That way you could add your own DB/redis-based matcher, that may require no loading at all.

Almost all the current methods in UserAgentParser::Parser are specific to the ua-parser database though, so they'd all be moved out to some browserscope-specific class.

BUT in saying all this, without another backend that someone would find useful (the memory one is just an example/test), then it's probably just premature abstraction.

The ability to set the YML pattern path was specifically for my use case of needing to bugfix the browserscope pattern DB without re-releasing the gem. Never for your own completely separate custom patterns. But if someone does need it, then let's do it.

[jnunemaker]

It is your gem, so feel free to do whatever you want and I'll deal with it. :) That said, passing a file path as a string to a class named parser feels dirty to me in my gut. It gets the job done and there is no other need beyond a file path right now, but the code just feels cleaner with the pattern loader. I'm cool with whatever though. Feel free to close mine and pull yours.

[toolmantim]

@jnunemaker yeah, it might not feel nice below the hood, but then I really don't like the following API for the only known (and potentially common) use case of our vendor'd ua-parser library being out of date:

path = '/some/path/to/regexes.yaml'
pattern_loader = UserAgentParser::FilePatternLoader.new(path)
parser = UserAgentParser::Parser.new(pattern_loader)

Open for other API suggestions though…

For now I'm going to merge this in, update things and re-release.

[bemurphy]

Old issue, but wanted to drop my thoughts in here.

I ran into the loading performance issue yesterday because I'd missed the bit in the readme about reusing a parser.

I think an injectable 'backend' of sorts like gh-4 had makes sense. Seems like it would be a safer bet if you are going to keep a global around for performance, it's better for it to be a small set of assuredly read-only code rather than a larger set, to avoid possible memory leaks and state bugs that could crop up.

@toolmantim are you using the global parser in production now? Curious if that is OK. I've got the loader monkeypatched for now to memoize in my project. Dig this gem btw, love the fact that I can get the same results if I need to port to a diff language.

[toolmantim]

@bemurphy yeah, UserAgentParser.parse is being used in some production for smaller scripts and such. Otherwise the instantiated version, to control at which point it's loaded. Having to monkeypatch is bad :( Did you want to gist the monkeypatch code?

[bemurphy]

@toolmantim here's the monkeypatch. It is by no means a good idea or a great guide (it also breaks the initializer since a different filepath on later calls would have no use), but using it for now until I figure out a better way:

https://gist.github.com/bemurphy/6408305

As for a real patch, probably have an injectable in place of the filepath, that can return the 3 pattern objects the initializer gets from #load_patterns here

seems like the triple patterns could be a tiny value object, could break down with #to_a or something

[toolmantim]

Why not just do something like the following?

require 'user_agent_parser'

module MyApplication

  # Instantiate the parser on load as it's quite expensive
  USER_AGENT_PARSER = UserAgentParser::Parser.new

  def self.user_agent_parser
    USER_AGENT_PARSER
  end

end

Perhaps we could simply add this to the readme, as it's a pretty common use case?

[bemurphy]

I was kind of worried about leaks so avoided that. But it's not much code and I haven't actually seen if it leaks, so probably not a valid concern.

Readme good idea on that, forking now.

[toolmantim]

Thanks @bemurphy!