+ Add support to "podspec" parameter for #452#453
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
chkuang-g
left a comment
There was a problem hiding this comment.
Thank you so much for contribute to this open-source repo!
I added a comment in your PR.
Please make sure this change works for your by building it first using
./gradlew buildplugin
And then install the local build to your project.
Shawn
| get { | ||
| string path; | ||
| if (!propertiesByName.TryGetValue("path", out path)) return ""; | ||
| if (!propertiesByName.TryGetValue("podspec", out path)){ |
There was a problem hiding this comment.
It is better to use a different property for "podspec".
Also, do similar override in PodFilePodLine
camphongdinh@a97b71d#diff-4e150db5d98cbbe99ab461893cc478b33377658b810b34ebc72f7549670ef482R154-R157
Note that from the Cocoapod doc, :podspec can point to an "http" url. So perhaps only expand the path only if it does not start with "http".
| /// <iosPods> | ||
| /// <iosPod name="name" | ||
| /// path="pathToLocal" | ||
| /// podspec="pathToLocalPodSpecJSON" |
There was a problem hiding this comment.
I think you can specify a url here as well
https://guides.cocoapods.org/syntax/podfile.html#pod
Perhaps use
/// podspec="pathToPodSpec"
* Rename podspec parameter to podspecPath to avoid conflicts *Support http for podspecPath
- Update comments for podspecPath
|
@chkuang-g Hi, I have updated the changes:
I have built the plugin and tested. It is working fine locally with this format in Dependencies.xml: Still I haven't managed to compile the source into a .tgz for use in Unity Package Manager. I will try again later. Please let me know if you need any other changes then. Thanks |
|
Thanks for the update. I will take a look today. |
chkuang-g
left a comment
There was a problem hiding this comment.
Also, could you also help to add the documentation here about the new properties?
Thank you!
| if (!propertiesByName.TryGetValue("podspecPath", out path)){ | ||
| if (!propertiesByName.TryGetValue("path", out path)) return ""; | ||
| } |
There was a problem hiding this comment.
As mentioned. It is better to have two different properties for path and podspecPath, and handle them separately later.
private string GetPathFromProperties(string name) {
string path;
if (!propertiesByName.TryGetValue(name, out path)) return "";
if (path.StartsWith("'") && path.EndsWith("'")) {
path = path.Substring(1, path.Length - 2);
}
return path;
}
public string LocalPath {
get {
return GetPathFromProperties("path");
}
}
public string PodSpecPath {
get {
return GetPathFromProperties("podspec");
}
}
See the next comment.
| "subspecs", | ||
| "path" | ||
| "path", | ||
| "podspecPath" |
There was a problem hiding this comment.
I think it is better to be just podspec to reduce confusion, since podspec is a real podfile properties like the other.
| if(outputPropertiesByName.ContainsKey("podspecPath")) { | ||
| // Check if it is a local folder or http:// | ||
| // If it is local folder add full path into OutputProperties | ||
| // If it is URI, kept as-is | ||
| if(Uri.IsWellFormedUriString(path, UriKind.Absolute)) { | ||
| outputPropertiesByName.Add("podspec", String.Format("'{0}'", path)); | ||
| } | ||
| else { | ||
| outputPropertiesByName.Add("podspec", String.Format("'{0}'", Path.GetFullPath(path))); | ||
| } | ||
| // Remove "path" parameter and temporary "podspecPath" parameter | ||
| outputPropertiesByName.Remove("path"); | ||
| outputPropertiesByName.Remove("podspecPath"); | ||
| } | ||
| else { | ||
| outputPropertiesByName["path"] = String.Format("'{0}'", Path.GetFullPath(path)); | ||
| } |
There was a problem hiding this comment.
Well, it is really nice to check redundancy here but I will leave that responsibility to the person who author Dependencies.xml, unless you are adding additional warning here when both properties exist.
I think it is much cleaner for you and your case now to just treat them as separate properties, like this
var podSpecPath = PodSpecPath;
if (!String.IsNullOrEmpty(podSpecPath)) {
if(Uri.IsWellFormedUriString(podSpecPath, UriKind.Absolute)) {
outputPropertiesByName.Add("podspec", String.Format("'{0}'", podSpecPath));
}
else {
outputPropertiesByName.Add("podspec", String.Format("'{0}'", Path.GetFullPath(podSpecPath)));
}
}
var localPath = LocalPath;
if (!String.IsNullOrEmpty(localPath)) {
outputPropertiesByName["path"] = String.Format("'{0}'", Path.GetFullPath(localPath));
}
883eedd to
5c6f23b
Compare
Add support to "podspec" parameter. If both "podspec" and "path" is present, it will prefer "podspec" over "path" for that specific pod.