Pkg compat #53
Pkg compat #53
Conversation
Merge pull request dart-archive#44 from dam0vm3nt/master
Minor fix in tests to get GlobalConfig rather than Future
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
I have signed the CLA |
|
k, tnx. |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
Yes I am okay with the commit I made being contributed to the project. |
|
All this just for a single line change :-) |
|
eh, the good news is that you have to do this only once for any google project. But the first time is cumbersome. |
|
True, but I can't help but see the funny side |
|
Um, I assume I've done what I need to do? |
|
Actually I don't know! I figure some google guy should approve your contribution ? |
|
Lets hope that's all that's needed. |
| @@ -148,8 +152,11 @@ void _generateImportStub( | |||
| /// generates a FileSummary for it. | |||
| Future<FileSummary> _parseFile(String inputPath, GlobalConfig globalConfig, | |||
| {bool ignoreFileErrors: false}) async { | |||
| var results = await Process.run( | |||
There was a problem hiding this comment.
can process_elements.sh be deleted now?
There was a problem hiding this comment.
I would tend to agree, get rid of it
There was a problem hiding this comment.
I thought I had deleted it! Now I've done it. Now I've done it, but I'm delaying the push until the review get finished to not invalidate other comments.
lib/src/config.dart
Outdated
| return path.join('packages', platformIndependentPath(parts[1])); | ||
| Uri uri = await config.packageResolver.resolveUri(filePath); | ||
| String p = platformIndependentPath(uri.toFilePath()); | ||
| p = p.replaceAll("%", r'%25'); // Reset url encoding |
There was a problem hiding this comment.
This part still seems sketchy to me?
There was a problem hiding this comment.
Not sure I understand, but certainly I think lines 213 and 214 could be removed and line 212 be modified to be
return platformIndependentPath(uri.toFilePath()).replaceAll("%", r'%25');
Or maybe put the string replacement
replaceAll("%", r'%25');
in the platformIndependentPath(...) function
There was a problem hiding this comment.
My question is why does the url encoding need to be reset? I am curious what the uri looks like before/after this line. I wouldn't expect any % in there at all intuitively. I haven't used the packageResolver package before but it seems like its giving a weird path here, and this is some sort of workaround?
There was a problem hiding this comment.
Actually I don't remember. Some time has passed. I will do some investigations
There was a problem hiding this comment.
ok, forget my previous comment. I had to undo the change to understand it.
I think the reason is that when the path is passed to the analyzer node script, it will get unescaped again by node parser .
The replaceAll actually could also be moved here just before passing it to the script.
|
@jakemac53 made some minor fixes. Do you know also how to clear the |
|
Yes I have and I've also said I'm happy for my one line commit to be used. If that's not clear, I reiterate, I'm happy for my commits to be used in this pull request |
support for
.packagesfile.