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

Brand model #10

Merged
merged 3 commits into from
Jan 12, 2015
Merged

Brand model #10

merged 3 commits into from
Jan 12, 2015

Conversation

commenthol
Copy link
Contributor

Adds Brand-Model parsing. Detects a lot of new Devices and Brands.
Tests are currently separate withing tests/test_device_brandmodel.yaml.

@commenthol
Copy link
Contributor Author

Build will NOT pass - this requires an update of the ref-impl and an npm publish first!

@tobie
Copy link
Contributor

tobie commented Nov 29, 2014

Mind splitting that PR up? Looks like it's addressing a bunch of different things (the spec, test cases, etc.).

@@ -1,72 +1,104 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep pull request focused on one topic.

@commenthol
Copy link
Contributor Author

Should be focused now.


- user_agent_string: 'AdsBot-Google-Mobile (+http://www.google.com/mobile/adsbot.html) Mozilla (iPhone; U; CPU iPhone OS 3 0 like Mac OS X) AppleWebKit (KHTML, like Gecko) Mobile Safari'
family: 'Spider'
family: 'Spider Smartphone'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to family: 'Spider'

@tobie
Copy link
Contributor

tobie commented Nov 29, 2014

Why add a new file instead of appending them to tests/test_device.yaml?

@commenthol
Copy link
Contributor Author

The testset is quite large (15799 testcases). As mentioned on the forum there was the idea of having a small and a big testset for the different language parsers to process.
If you want to have all in a single file - no problem. Please let me know.

@tobie
Copy link
Contributor

tobie commented Dec 3, 2014

The testset is quite large (15799 testcases). As mentioned on the forum there was the idea of having a small and a big testset for the different language parsers to process.

Yeah, that's an orthogonal issue. The "small" test suite should just exercise proper implementation of the spec and regex file (possibly even through made-up UA strings).

If you want to have all in a single file - no problem. Please let me know.

Yes please. I'd like to have 1 file for each one of UA, OS and device.

@commenthol
Copy link
Contributor Author

All tests are now in test_device.yaml.

@commenthol
Copy link
Contributor Author

ping...

@tobie
Copy link
Contributor

tobie commented Jan 10, 2015

Doesn't seem to merge properly, mind looking into it?

@commenthol
Copy link
Contributor Author

Travis CI still uses uap-ref-impl@0.1.0 (See line 111). In order to pass all tests, npm requires an update of "uap-ref-impl" with the version 0.2.0 checked in.
Can you please be so kind to checkout the latest "uap-ref-impl" and publish on npm? (See ua-parser/uap-ref-impl#6)

Paste this into your terminal...

# clone repo
git clone https://github.com/ua-parser/uap-ref-impl.git
cd uap-ref-impl
# pack the stuff
npm pack
# take a look whats inside
tar tvzf uap-ref-impl-0.2.0.tgz
# ... and out with it ... ;)
npm publish

@tobie
Copy link
Contributor

tobie commented Jan 12, 2015

Oh, sorry, I thought there was a merge conflict, not a test suite issue. My bad.

tobie added a commit that referenced this pull request Jan 12, 2015
@tobie tobie merged commit 22ef8a1 into ua-parser:master Jan 12, 2015
@commenthol commenthol deleted the brand-model branch February 11, 2015 19:24
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.

2 participants