Skip to content

Remove one TODO #76275

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.

Sign up for GitHub

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

Jump to bottom
Merged
fluttergithubbot merged 7 commits into from
Mar 11, 2021
Merged

Remove one TODO #76275

fluttergithubbot merged 7 commits into from
Mar 11, 2021

Conversation

@Abhishek01039
Copy link
Contributor

@Abhishek01039 Abhishek01039 commented Feb 18, 2021 •
edited
Loading

it's fixed #8986 is resolved.

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 listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • 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 c: contributor-productivity Team-specific productivity, code health, technical debt. label Feb 18, 2021
@google-cla google-cla bot added the cla: yes label Feb 18, 2021
Hixie
Hixie reviewed Feb 18, 2021
View reviewed changes
dev/integration_tests/channels/android/app/build.gradle Outdated
cruncherEnabled false
}

}
Copy link
Contributor

@Hixie Hixie Feb 18, 2021

Choose a reason for hiding this comment

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

can we remove the entire aaptOptions section? (I'm not sure, I'm not a gradle expert.)

Copy link
Contributor Author

@Abhishek01039 Abhishek01039 Feb 19, 2021

Choose a reason for hiding this comment

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

Do you have to ask anyone for this @Hixie?

Copy link
Contributor

@Hixie Hixie Feb 25, 2021

Choose a reason for hiding this comment

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

I was asking you :-)
Maybe someone in #hackers knows.

Copy link
Contributor Author

@Abhishek01039 Abhishek01039 Mar 1, 2021

Choose a reason for hiding this comment

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

Hello @Hixie
can you please ask any member of your team?
Thanks

Copy link
Contributor

@daadu daadu Mar 5, 2021

Choose a reason for hiding this comment

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

This can be removed as far as I know. This will also pass the pipeline because "white spaced line" was causing it.

@Abhishek01039 Abhishek01039 changed the title Removed one TODO Remove one TODO Feb 18, 2021
@flutter-dashboard flutter-dashboard bot added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Feb 18, 2021
@Abhishek01039 Abhishek01039 requested a review from Hixie March 5, 2021 14:06
@Hixie
Copy link
Contributor

Hixie commented Mar 5, 2021

I'm the wrong person to review this, but since the TODO was added by @goderbauer, maybe they are the right person?

BTW you have a test failing because some of the gradle files have trailing spaces.

goderbauer
goderbauer reviewed Mar 5, 2021
View reviewed changes
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 once merge conflict is resolved and tests are passing

dev/integration_tests/android_semantics_testing/android/app/build.gradle Outdated
}
}

aaptOptions {
Copy link
Member

@goderbauer goderbauer Mar 5, 2021

Choose a reason for hiding this comment

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

I believe we can just remove the entire "aaptOptions" section.

daadu
daadu reviewed Mar 6, 2021
View reviewed changes
dev/integration_tests/image_loading/android/app/bin/build.gradle Outdated
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

def localProperties = new Properties()
Copy link
Contributor

@daadu daadu Mar 6, 2021

Choose a reason for hiding this comment

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

Is this file added by mistake?

Copy link
Contributor Author

@Abhishek01039 Abhishek01039 Mar 7, 2021

Choose a reason for hiding this comment

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

Oh yes.
Thanks for letting me know.

dev/integration_tests/image_loading/android/app/build.gradle Outdated
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

def localProperties = new Properties()
Copy link
Contributor

@daadu daadu Mar 6, 2021

Choose a reason for hiding this comment

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

Is this file added by mistake?

goderbauer
goderbauer approved these changes Mar 8, 2021
View reviewed changes
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

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Mar 8, 2021

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

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

@goderbauer goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label Mar 8, 2021
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Mar 11, 2021

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

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

@pingbird
Copy link
Member

pingbird commented Mar 11, 2021

Tests are passing now

@fluttergithubbot fluttergithubbot merged commit bc9d0e0 into flutter:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goderbauer goderbauer goderbauer approved these changes

@Hixie Hixie Awaiting requested review from Hixie

+1 more reviewer

@daadu daadu daadu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Gradle APK build crashes in AAPT cruncher step

7 participants

@Abhishek01039 @Hixie @fluttergithubbot @pingbird @goderbauer @daadu @Piinks