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

Fix ESPN Radio podcast catcher regex #305

Merged
merged 3 commits into from
Mar 21, 2018
Merged

Fix ESPN Radio podcast catcher regex #305

merged 3 commits into from
Mar 21, 2018

Conversation

asuhan
Copy link
Contributor

@asuhan asuhan commented Feb 24, 2018

Make it more precise and compatible with Boost.Regex syntax as well.

Make it more precise and compatible with Boost.Regex syntax as well.
@sunknudsen
Copy link
Contributor

@asuhan Hi, could you please add a test for your ESPN Radio regex to https://github.com/ua-parser/uap-core/blob/master/tests/test_ua.yaml? Thanks!

@asuhan
Copy link
Contributor Author

asuhan commented Mar 9, 2018

Hello, it's already covered, for example: https://github.com/ua-parser/uap-core/blob/master/test_resources/podcasting_user_agent_strings.yaml#L364. My change doesn't break these tests and works with C++ as well (for reference: ua-parser/uap-cpp#18).

@commenthol
Copy link
Contributor

LGTM

@silviucpp
Copy link

any idea when this fix will be merged ?

@sunknudsen
Copy link
Contributor

@asuhan @silviucpp Hey guys, the tests in the test_resources are legacy. Can you please add a test for your ESPN Radio regex to https://github.com/ua-parser/uap-core/blob/master/tests/test_ua.yaml? Once done, I will merge your pull request. Thanks!

@asuhan
Copy link
Contributor Author

asuhan commented Mar 21, 2018

@sunknudsen As I've already pointed out, https://github.com/ua-parser/uap-core/blob/master/test_resources/podcasting_user_agent_strings.yaml#L364 contains tests for the regex I'm changing. They still pass; they look adequate. I have no idea how else to test this change. Does this mean porting them over to test_ua.yaml?

@asuhan
Copy link
Contributor Author

asuhan commented Mar 21, 2018

I've simply copied the tests to tests/test_ua.yaml, no idea if they pass since the PR has conflicts.

@sunknudsen sunknudsen merged commit fc570f3 into ua-parser:master Mar 21, 2018
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.

4 participants