-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[tool] XCResult also parses "url" in xcresult bundle #95054
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
|
|
||
| import '../../src/common.dart'; | ||
| import '../../src/fake_process_manager.dart'; | ||
| import 'xcresult_test_data.dart'; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a note for you, not intended to be checked in... 🙂
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.
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.