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

Handle condition where skip_spaces could put the index out of range. #1085

Merged
merged 1 commit into from
Sep 1, 2019

Conversation

eahrold
Copy link

@eahrold eahrold commented Sep 1, 2019

@msqar
Copy link

msqar commented Sep 1, 2019

Good job, man. I hope this gets merged asap.

@msqar
Copy link

msqar commented Sep 1, 2019

@msand no mean to disrespect by tagging you but this PR fixes something really important, it's literally fixing this bug that breaks the library from being used. Hope we can merge it as soon as possible. Thanks in advance.

@msand msand merged commit 480b082 into software-mansion:develop Sep 1, 2019
@msand
Copy link
Collaborator

msand commented Sep 1, 2019

Published v9.8.2
@msqar In the future, no need to rush these thing, you're able to use forks/specific commits without them being in the master branch / published to npm.

@msqar
Copy link

msqar commented Sep 1, 2019

@msand oh thanks, didn't know. :) thanks for your time. And sorry to bother!

@@ -39,6 +39,10 @@ Path parse(String data) {
while (i < l) {
skip_spaces();

if (i >= l) {
break
Copy link

@msqar msqar Sep 1, 2019

Choose a reason for hiding this comment

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

Was lint complaining about the missing ";" when u fixed this? It is complaining on mine D:

@msqar
Copy link

msqar commented Sep 1, 2019

@eahrold Oops, lint complaining about a missing ";" in the java file. Do you disable lint?

@eahrold
Copy link
Author

eahrold commented Sep 1, 2019

@msqar you're absolutely right needs the ;
My mistake. I ran the npm run lint but it doesn't catch native code.

> react-native-svg $ npm run lint

> react-native-svg@9.8.1 lint ../Code/react-native-svg
> eslint ./


../Code/react-native-svg/lib/extract/extractColor.js
  153:12  warning  'name' is defined but never used  no-unused-vars

../Code/react-native-svg/lib/util.js
  3:14  warning  'key' is defined but never used  no-unused-vars

✖ 2 problems (0 errors, 2 warnings)

@msand
Copy link
Collaborator

msand commented Sep 1, 2019

Those warnings are a bug in eslint. If the native code compiles it should be enough. Assumed you had done that.

@eahrold
Copy link
Author

eahrold commented Sep 1, 2019

Those warnings are a bug in eslint. If the native code compiles it should be enough. Assumed you had done that.

Unfortunately, I did not. I threw the changes together late last night without even intending to do a PR, and they were so small I thought "why not", and then did the pr to develop.

@msand It was careless of me, and I apologize. I hope it didn't bum out your Sunday too much.

@msand
Copy link
Collaborator

msand commented Sep 1, 2019

No worries, takes seconds to cut a release anyway, and noone else seems to have experienced the issue, no hard feelings :) Thanks for the fix!

@msqar
Copy link

msqar commented Sep 1, 2019

Thanks for the fix guys :)

@msand
Copy link
Collaborator

msand commented Sep 1, 2019

Btw, this was certainly on me, I should've added continous integration with unit and end to end tests long ago, or tried using the changes at the very least. I set up a e2e testing prototype this spring, and this reminds me I need finish that up.

@synoumar
Copy link

synoumar commented Dec 2, 2019

I am getting the same error of ''Error while updating property 'd' of a View managed by: RNSVGPath' while I create an array of String data

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