-
Notifications
You must be signed in to change notification settings - Fork 371
Fixing https://github.com/googlesamples/unity-jar-resolver/issues/48 #49
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
Conversation
googlebot
commented
May 6, 2017
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
📝 Please visit https://cla.developers.google.com/ to sign.
Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.
- If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
- If you signed the CLA as a corporation, please let us know the company's name.
I signed it!
ca490f5 to
f244e7d
Compare
googlebot
commented
May 6, 2017
CLAs look good, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependency.BestVersion should be the version string so in both cases this will be either a version string or an empty string see https://github.com/googlesamples/unity-jar-resolver/blob/master/source/JarResolverLib/src/Google.JarResolver/Dependency.cs#L158
Why did you need to change this to extract the version from the filename? Are you seeing a file name sneak into this version field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why you need this. The previous code attempted to extract the version from the filename and if the match succeeds then jumps into the block below and pulls the version from match.Groups[1].Value. Did you see a problem in this code path or was this just a clean up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stack trace reported in bug #48 starts here and bubbles up through to Resolver1_1.AarPathToPackageName where - from your report - the filename was null and therefore caused a null reference exception. Which suggests https://github.com/googlesamples/unity-jar-resolver/blob/master/source/PlayServicesResolver/src/ResolverVer1_1.cs#L723 is returning a null path (as artifactPath) instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch.
Though, I don't see how this fixes #48 given the reported stack trace. Could you add more details to the commit message that describes what the problem is and how you fixed it?
See #48 (comment)
Ah ok, I see now. Any chance you could update the commit message to reflect the comment you added here #48 (comment) ?
Using the same rule (and the same function) for extracting a version code when comparing libraries versions, as used to generate file names when storing them in Plugins/Android folder, so repetitive checks for versions with -alpha like postfixes don't misdetect them as a new version each time.
f244e7d to
e49d009
Compare
@stewartmiles ,
Sure. Done.
Thank you.
Fix for #48