Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Jul 13, 2021

the generate_dart_ui target is redundant with the sky_engine target.

We should just point the analyzer to that one to avoid the extra file copies.

For reasons I do not understand, analyzing the generate_dart_ui target fails to catch some doc-comments missing in semantics.dart, but catches them correctly in the updated location.

I'm making generate_dart_ui an empty target until we can remove references from it in CI.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@dnfield dnfield requested a review from chunhtai July 13, 2021 18:48
@google-cla google-cla bot added the cla: yes label Jul 13, 2021
@dnfield dnfield requested a review from zanderso July 13, 2021 18:49
final TextRange range;

// Returns a copy of this atttribute with the given range.
/// Returns a copy of this atttribute with the given range.
Copy link
Contributor

Choose a reason for hiding this comment

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

// is appropriate here since as written this is not adequate documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we need to update the text, because this fails analysis requiring public members be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chunhtai @goderbauer - I've updated the text here, PTAL to make sure it makes sense.

bindings_output_dir = "$root_gen_dir/sky/bindings"

copy("generate_dart_ui") {
# TODO(dnfield): Remove this when recipes are updated to not use it.
Copy link
Contributor

@chunhtai chunhtai Jul 13, 2021

Choose a reason for hiding this comment

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

which recipes are this refering to? Can you also file an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The LUCI recipes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's include the link in the todo so we know when to clean up this once it merges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Hixie
Copy link
Contributor

Hixie commented Jul 13, 2021

test-exempt: is a test

});

// The range of the text to which this attribute applies.
/// The range of the text to which this attribute applies.
Copy link
Contributor

Choose a reason for hiding this comment

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

oops thanks for fixing this

@dnfield dnfield force-pushed the fix_dart_analysis branch from 39a2c0c to 9b1072e Compare July 13, 2021 18:56
@dnfield
Copy link
Contributor Author

dnfield commented Jul 13, 2021

This PR mistakenly contained changes from another branch I'm working on - I've reverted that.

/// For example, the [LocaleStringAttribute] specifies a [Locale] for its
/// range of characters. Copying it will result in a new
/// [LocaleStringAttribute] that has the same locale but an updated
/// [TextRange].
Copy link
Contributor

Choose a reason for hiding this comment

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

This doc looks good to me

@dnfield dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 13, 2021
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Docs LGTM

@fluttergithubbot fluttergithubbot merged commit 0ae9b10 into flutter:master Jul 13, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 14, 2021
moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes needs tests waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants