Skip to content
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

Remove the defaults for textBaseline #68646

Merged
merged 1 commit into from Oct 29, 2020
Merged

Conversation

Hixie
Copy link
Contributor

@Hixie Hixie commented Oct 20, 2020

We can't know what the correct default is. It depends entirely on the application's locale.

cc @tvolkert @ramnavan

Reverts #60586, #61425. See also #58053.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 20, 2020
@google-cla google-cla bot added the cla: yes label Oct 20, 2020
try {
Table(defaultVerticalAlignment: TableCellVerticalAlignment.baseline);
throw TestFailure('Table should throw when missing necessary baseline information.');
} on AssertionError catch (error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of throwing in the test and catching, could we make use of expect and throwsA methods are mentioned here and here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out how to make throwsA verify the text of the exception. Can you elaborate on what you'd like it to look like?

Copy link
Member

Choose a reason for hiding this comment

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

You can use .having for that:

expect(
() => describeEnum('Hello World'),
throwsA(isAssertionError.having(
(AssertionError e) => e.message,
'message',
'The provided object "Hello World" is not an enum.'),
),
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

(this is why my ideal test framework has no expect and no matchers. Just use regular Dart. No need to learn crazy APIs when regular code can do the same thing in fewer lines of code, no shame in not knowing the random test library APIs. :-P)

@ramnavan
Copy link
Contributor

@Hixie - changes makes sense to me. added a minor suggestion to the test.

@Hixie Hixie force-pushed the baseline branch 2 times, most recently from 94bd29d to bb2023f Compare October 22, 2020 02:36
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

We can't know what the correct default is. It depends entirely on the application's locale.
@Hixie
Copy link
Contributor Author

Hixie commented Oct 29, 2020

applied suggested fix, will land on green

@fluttergithubbot
Copy link
Contributor

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants