[tool] XCResult also parses "url" in xcresult bundle #95054
[tool] XCResult also parses "url" in xcresult bundle #95054fluttergithubbot merged 7 commits intoflutter:masterfrom
Conversation
|
|
||
| import '../../src/common.dart'; | ||
| import '../../src/fake_process_manager.dart'; | ||
| import 'xcresult_test_data.dart'; |
There was a problem hiding this comment.
Moving the data into a separate file to prepare for #94747.
| final List<String> topComponents = url.split('#'); | ||
| final String filePath = topComponents.first; | ||
| if (topComponents.length == 1) { | ||
| return filePath; |
There was a problem hiding this comment.
This probably won't happen. Since it is not a json that we maintain, I'm not sure all the possibilities that could be. So just in case there are no parameters, we can show the file path at least.
| /// | ||
| /// This is a re-formatted version of the "url" value in the json. | ||
| /// The format looks like <file location>:<StartingLineNumber>:<StartingColumnNumber>. | ||
| final String location; |
There was a problem hiding this comment.
I think this should be nullable, an empty string isn't a valid location and shouldn't be passed to a Uri() parser, for example. If it's nullable, you would get an error if you tried to convert it to a Uri, but if it's empty it would happily give you a garbage value. The caller doesn't have to understand that an empty string is special, it can just check for null, that's what null is for. 🙂
Ditto for subType or anything else using that pattern.
| // A typical location url string looks like file:///foo.swift#CharacterRangeLen=0&EndingColumnNumber=82&EndingLineNumber=7&StartingColumnNumber=82&StartingLineNumber=7. | ||
| // | ||
| // This function converts it to something like: file:///foo.swift:<StartingLineNumber>:<StartingColumnNumber>. | ||
| String _convertUrlToLocationString(String url){ |
There was a problem hiding this comment.
Nit
| String _convertUrlToLocationString(String url){ | |
| String _convertUrlToLocationString(String url) { |
|
|
||
| // A typical location url string looks like file:///foo.swift#CharacterRangeLen=0&EndingColumnNumber=82&EndingLineNumber=7&StartingColumnNumber=82&StartingLineNumber=7. | ||
| // | ||
| // This function converts it to something like: file:///foo.swift:<StartingLineNumber>:<StartingColumnNumber>. |
There was a problem hiding this comment.
We should leave off the file scheme and just show the path.
| if (url.isEmpty) { | ||
| return ''; | ||
| } | ||
| final List<String> topComponents = url.split('#'); |
There was a problem hiding this comment.
We shouldn't parse Uris by hand. Unfortunately Xcode decided to put the query key values into the fragment, but we can swap them:
// A typical location url string looks like file:///foo.swift#CharacterRangeLen=0&EndingColumnNumber=82&EndingLineNumber=7&StartingColumnNumber=82&StartingLineNumber=7.
//
// This function converts it to something like: foo.swift:<StartingLineNumber>:<StartingColumnNumber>.
String? _convertUrlToLocationString(String url) {
final Uri? fragmentLocation = Uri.tryParse(url);
// Maybe log something here
if (fragmentLocation == null) {
return null;
}
// Parse the fragment as a query of key-values:
final Uri fileLocation = Uri(
path: fragmentLocation.path,
query: fragmentLocation.fragment,
);
String? startingLineNumber = fileLocation.queryParameters['StartingLineNumber'] ?? '';
if (startingLineNumber.isNotEmpty) {
startingLineNumber = ':$startingLineNumber';
}
String startingColumnNumber = fileLocation.queryParameters['StartingColumnNumber'] ?? '';
if (startingColumnNumber.isNotEmpty) {
startingColumnNumber = ':$startingColumnNumber';
}
return '${fileLocation.path}$startingLineNumber$startingColumnNumber';
}| expect(result.issues.first.type, XCResultIssueType.error); | ||
| expect(result.issues.first.subType, 'Semantic Issue'); | ||
| expect(result.issues.first.message, "Use of undeclared identifier 'asdas'"); | ||
| expect(result.issues.first.location, 'file:///Users/m/Projects/test_create/ios/Runner/AppDelegate.m:7:56'); |
There was a problem hiding this comment.
| expect(result.issues.first.location, 'file:///Users/m/Projects/test_create/ios/Runner/AppDelegate.m:7:56'); | |
| expect(result.issues.first.location, '/Users/m/Projects/test_create/ios/Runner/AppDelegate.m:7:56'); |
| final String? location; | ||
|
|
||
| /// Warnings when constructing the issue object. | ||
| final List<String> warnings; |
There was a problem hiding this comment.
Let the caller decide what to do with the warnings (e.g. log trace)
|
@jmagman Updated per review comments. PTAL |
jmagman
left a comment
There was a problem hiding this comment.
LGTM when // Maybe log something here is removed.
| // This function converts it to something like: /foo.swift:<StartingLineNumber>:<StartingColumnNumber>. | ||
| String? _convertUrlToLocationString(String url) { | ||
| final Uri? fragmentLocation = Uri.tryParse(url); | ||
| // Maybe log something here |
There was a problem hiding this comment.
This was a note for you, not intended to be checked in... 🙂
There was a problem hiding this comment.
oh, the copy-paste :p
|
This pull request is not suitable for automatic merging in its current state.
|
XCResultParser will also parse the "url" and convert them into a more readable string:
Fixes #95036
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.