Skip to content

Conversation

@cyanglaz
Copy link
Contributor

XCResultParser will also parse the "url" and convert them into a more readable string:

From
file:///foo.swift#CharacterRangeLen=0&EndingColumnNumber=82&EndingLineNumber=7&StartingColumnNumber=82&StartingLineNumber=7.
To
file:///foo.swift:7:82

Fixes #95036

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Dec 10, 2021
@cyanglaz cyanglaz requested a review from jmagman December 10, 2021 23:37
@cyanglaz cyanglaz mentioned this pull request Dec 10, 2021
8 tasks

import '../../src/common.dart';
import '../../src/fake_process_manager.dart';
import 'xcresult_test_data.dart';
Copy link
Contributor Author

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;
Copy link
Contributor Author

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;
Copy link
Member

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){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

Suggested change
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>.
Copy link
Member

@jmagman jmagman Dec 10, 2021

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('#');
Copy link
Member

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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;
Copy link
Contributor Author

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)

@cyanglaz
Copy link
Contributor Author

@jmagman Updated per review comments. PTAL

Copy link
Member

@jmagman jmagman left a 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
Copy link
Member

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... 🙂

Copy link
Contributor Author

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

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Mac build_tests_2_4 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot merged commit 70fea6d into flutter:master Dec 14, 2021
@cyanglaz cyanglaz deleted the xcresult_parse_url branch December 14, 2021 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

xcresult parser shows location of compilation error.

3 participants