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

updated test cases and family name for SznProhlizec #304

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

panther7
Copy link
Contributor

@panther7 panther7 force-pushed the master branch 2 times, most recently from 4ccb6c1 to 28d096c Compare February 21, 2018 09:55
@panther7 panther7 changed the title added support patch-minor for SznProhlizec updated SznProhlizec Feb 21, 2018
@panther7
Copy link
Contributor Author

Thank you for future merge ;-)

@janRucka
Copy link

janRucka commented Mar 1, 2018

+1
Good work

@sunknudsen
Copy link
Contributor

@panther7 Thanks for the pull request. :)

Why not use (SznProhlizec)\/(\d+)\.(\d+)\.(\d+)?

According to your unit tests in tests/test_ua.yaml, this simplified regex would works just as well.

Perhaps you are addressing other user agent strings that are not part of the unit tests.

@panther7
Copy link
Contributor Author

panther7 commented Mar 9, 2018

@sunknudsen Because, I don't want "-12345" in patch version. This doesn't affect public releases but internal releases can have this suffix. Thanks for understand ;-)

(SznProhlizec)\/(\d+)\.(\d+)\.(\d+) dismatch SznProhlizec/4.4i

@sunknudsen
Copy link
Contributor

@panther7 According to spec \d+ will only match digits from 0-9 so the match would stop before the -.

https://regexr.com/3lvd6

@commenthol Should contributors add unit tests for internal release use cases such as #304 (comment) to tests/test_ua.yaml?

@sunknudsen Because, I don't want "-12345" in patch version. This doesn't affect public releases but internal releases can have this suffix. Thanks for understand ;-)

(SznProhlizec)/(\d+).(\d+).(\d+) dismatch SznProhlizec/4.4i

@panther7
Copy link
Contributor Author

panther7 commented Mar 9, 2018

@sunknudsen ... :-/

(SznProhlizec)/(\d+).(\d+).(\d+) dismatch SznProhlizec/4.4i

But I could use (SznProhlizec)\/(\d+)\.(\d+)(?:\.(\d+))? ...but this is same like before :)

@sunknudsen
Copy link
Contributor

@panther7 Thanks for updating the test. Waiting for the feedback of @commenthol about documenting internal use cases before merging your pull request. I'm new to the project so want to make sure I respect conventions. Have a nice day!

@panther7 panther7 force-pushed the master branch 2 times, most recently from 01f905b to e2f3a7b Compare March 9, 2018 13:50
@panther7 panther7 changed the title updated SznProhlizec updated test cases for SznProhlizec Mar 9, 2018
@panther7 panther7 changed the title updated test cases for SznProhlizec updated test cases and family name for SznProhlizec Mar 9, 2018
@commenthol
Copy link
Contributor

Commit 45fb215 looks good to me. Well (\d+)\.(\d+)(?:\.(\d+))? is often used if versions consists of either two or three digits.

@sunknudsen sunknudsen merged commit 60657b6 into ua-parser:master Mar 20, 2018
@@ -270,7 +270,7 @@ user_agent_parsers:

# Seznam.cz browser (based on WebKit)
- regex: '(SznProhlizec)/(\d+)\.(\d+)(?:\.(\d+))?'
family_replacement: 'Seznam.cz'
family_replacement: 'Seznam prohlížeč'
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is the first instance of a non-ASCII family value. This will cause issues for those wanting to use the family value in a HTTP header as they are only US-ASCII. Would it be possible to have an ASCII based alternative value as well? E.G. family_replacement_ascii: 'Seznam.cz'

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.

5 participants