Skip to content

Removing Unicode EN+Space for Space #29

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

Closed
wants to merge 16 commits into from

Conversation

vincentkoc
Copy link
Contributor

Removing references of u\2002 in data files and unit tests. Originally reported on PHP library WhichBrowser/Parser-PHP#96 - And same PR built for PHP ported to JS library WhichBrowser/Parser-PHP#106

Removing references of u\2002 in data files and unit tests. Originally reported on PHP library WhichBrowser/Parser-PHP#96 - And same PR built for PHP ported to JS library WhichBrowser/Parser-PHP#106
@vincentkoc
Copy link
Contributor Author

Hey, on this PR. Test pasted by Travis CI. However it seems to be causing an issue on the codecoverage report for some odd reason. See screenshot, please review CI log or re-run/fix CI issue before dismissing this PR.

screenshot 2019-01-20 23 35 53

Disconnecting Code Coverage from Tests
Adding logical versions and failure ability on nightly builds
@vincentkoc
Copy link
Contributor Author

Please see PR #30 for resolving the Travis CI issues

Vincent Koc and others added 7 commits January 21, 2019 22:45
…-1.16.0

Bump prettier from 1.13.7 to 1.16.0
….88.0

Bump request from 2.87.0 to 2.88.0
@mariotsi
Copy link
Member

PR #30 merged. Can you rebase your PR on top of it so we can test it via Travis? Thanks

Vincent Koc and others added 3 commits January 22, 2019 23:35
Resolving NodeJS Travis CI Issues (#30)
Removing references of u\2002 in data files and unit tests. Originally reported on PHP library WhichBrowser/Parser-PHP#96 - And same PR built for PHP ported to JS library WhichBrowser/Parser-PHP#106
@vincentkoc
Copy link
Contributor Author

@mariotsi i have rebased the PR and tests for the unicode fix are now passing

@mariotsi
Copy link
Member

@koconder just realised that I cannot merge it because the data files are automatically generated by downloading and adapting the PHP version.
If @NielsLeenheer is gonna change the PHP version of those files the change will be reflected in the Javascript version by just running a command.
Otherwise the downloader script has to be updated. Feel free to do it or update the CR in order to remove the changes on the data files and I'll do the change on the scripts.

Thanks!

@vincentkoc
Copy link
Contributor Author

@mariotsi just FYI i have made a PR to the PHP Build as well, see WhichBrowser/Parser-PHP#106

Let me know how best to proceed, if we need to close this PR if the PHP data-files is fixed. Or if its worth me updating the "builder" to strip unicode.

@mariotsi
Copy link
Member

I've to check which is the source of data of the PHP version. If I remember well even those are automatically generate. I will check it this evening and let you know

@vincentkoc
Copy link
Contributor Author

@mariotsi any updates for me to rectify my PR's?

@mariotsi
Copy link
Member

Yes, sorry I've checked but I didn't replied to you. @NielsLeenheer seems busy at the moment, so in order to don't block this PR I would like to remove the change from the data files, keep it the test files and update the script in order to strip the characters in a way that won't break when PHP changes are merged. Let me know if you feel comfortable in doing so otherwise I'll do it and you can rebase on top of it 💪

@vincentkoc
Copy link
Contributor Author

@mariotsii might need a hand with the changes you have asked, my node.js skills are not the best and I dont know how the functions for building so might be best handled by someone more versed on the repo. Or we just hold tight for the PR merge on the PHP version.

@mariotsi
Copy link
Member

mariotsi commented Feb 1, 2019

@koconder No worries. This weekend I'll add the code for stripping the character so can rebase on top of it. I'm not so keen to wait for the PHP version because often @NielsLeenheer is busy and cannot merge PRs/comment on issue. Additionally the change I'm gonna make will not break when he will merge 🤙

mariotsi added a commit that referenced this pull request Feb 6, 2019
@mariotsi
Copy link
Member

mariotsi commented Feb 6, 2019

This PR has been merged to the branch Strip-EN+Space-from-models-and-profiles and is now part of #33

@mariotsi mariotsi closed this Feb 6, 2019
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