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

Unnecessary new #20138

Merged
merged 8 commits into from Sep 12, 2018

Conversation

Projects
None yet
4 participants
@a14n
Contributor

a14n commented Aug 2, 2018

No description provided.

@googlebot googlebot added the cla: yes label Aug 2, 2018

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Aug 2, 2018

Contributor

@Hixie

I've push the 3 commits to split changes:

  • the first commit is the result of a script to remove new pointed out by the unnecessary_new lint
  • the second commit to fix analysis
  • the last one to fix tests
Contributor

a14n commented Aug 2, 2018

@Hixie

I've push the 3 commits to split changes:

  • the first commit is the result of a script to remove new pointed out by the unnecessary_new lint
  • the second commit to fix analysis
  • the last one to fix tests

@a14n a14n referenced this pull request Aug 6, 2018

Open

Meta-issue: Update all code to use optional new/const #18604

3 of 4 tasks complete
@Hixie

This comment has been minimized.

Show comment
Hide comment
@Hixie

Hixie Aug 14, 2018

Contributor

It looks like the ## Sample code sections didn't get updated by this. I don't understand how this passed the analysis step... are we using the wrong set of lints for the sample code analysis?

Contributor

Hixie commented Aug 14, 2018

It looks like the ## Sample code sections didn't get updated by this. I don't understand how this passed the analysis step... are we using the wrong set of lints for the sample code analysis?

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Aug 28, 2018

Contributor

It looks like the ## Sample code sections didn't get updated by this. I don't understand how this passed the analysis step... are we using the wrong set of lints for the sample code analysis?

Looking at dev/bots/analyze-sample-code.dart it seems that no lints are used to analyze sample code.

We could update dev/bots/analyze-sample-code.dart in an other PR.

Contributor

a14n commented Aug 28, 2018

It looks like the ## Sample code sections didn't get updated by this. I don't understand how this passed the analysis step... are we using the wrong set of lints for the sample code analysis?

Looking at dev/bots/analyze-sample-code.dart it seems that no lints are used to analyze sample code.

We could update dev/bots/analyze-sample-code.dart in an other PR.

@Hixie

This comment has been minimized.

Show comment
Hide comment
@Hixie

Hixie Aug 28, 2018

Contributor

Ah, bummer. Yeah, we should definitely run lints in that script!

Contributor

Hixie commented Aug 28, 2018

Ah, bummer. Yeah, we should definitely run lints in that script!

@Hixie

This comment has been minimized.

Show comment
Hide comment
@Hixie

Hixie Aug 28, 2018

Contributor

Maybe we should make that change first, actually, so that we are consistent throughout about new.

Contributor

Hixie commented Aug 28, 2018

Maybe we should make that change first, actually, so that we are consistent throughout about new.

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Aug 28, 2018

Contributor

Actually there are already places where new or const have been omitted. Moreover unnecessary_const is already activated in code but not in sample. So I don't think the order of changes is important.

Contributor

a14n commented Aug 28, 2018

Actually there are already places where new or const have been omitted. Moreover unnecessary_const is already activated in code but not in sample. So I don't think the order of changes is important.

@Hixie

This comment has been minimized.

Show comment
Hide comment
@Hixie

Hixie Aug 28, 2018

Contributor

The omission of new in various places (so that we're inconsistent) is a problem.

Contributor

Hixie commented Aug 28, 2018

The omission of new in various places (so that we're inconsistent) is a problem.

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Sep 5, 2018

Contributor

First step for lint on sample codes done in #21155 ( review welcome ;) )

Contributor

a14n commented Sep 5, 2018

First step for lint on sample codes done in #21155 ( review welcome ;) )

a14n added some commits Sep 7, 2018

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Sep 7, 2018

Contributor

CI ok, PTAL

Contributor

a14n commented Sep 7, 2018

CI ok, PTAL

@a14n a14n merged commit d927c93 into flutter:master Sep 12, 2018

11 checks passed

WIP ready for review
Details
analyze
Details
cla/google All necessary CLAs are signed
codelabs-build-test
Details
docs
Details
tests-linux
Details
tests-macos
Details
tests-windows
Details
tool_tests-linux
Details
tool_tests-macos
Details
tool_tests-windows
Details

@a14n a14n deleted the a14n:unnecessary_new branch Sep 12, 2018

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Sep 12, 2018

Contributor

@Hixie : Oops! I'm really sorry! I mixed your RSLGTM on flutter/plugins#701 with this PR and I've merged it :-(
Should I revert the merge for this current PR (d927c93)?

Contributor

a14n commented Sep 12, 2018

@Hixie : Oops! I'm really sorry! I mixed your RSLGTM on flutter/plugins#701 with this PR and I've merged it :-(
Should I revert the merge for this current PR (d927c93)?

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Sep 12, 2018

Contributor

BTW: what's the workflow to follow in the case of such a mistake?

Contributor

a14n commented Sep 12, 2018

BTW: what's the workflow to follow in the case of such a mistake?

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Sep 12, 2018

Contributor

@goderbauer any advice as you seem online?

Contributor

a14n commented Sep 12, 2018

@goderbauer any advice as you seem online?

@goderbauer

This comment has been minimized.

Show comment
Hide comment
@goderbauer

goderbauer Sep 12, 2018

Member

I don't think we have a process for this. You could just revert this. However, as I understand it, the only remaining problem is that we are still showing new in various sample codes? If you can fix that quickly you can send it to me for review and we should be good as it is? Otherwise, I'd probably revert this PR for now.

Member

goderbauer commented Sep 12, 2018

I don't think we have a process for this. You could just revert this. However, as I understand it, the only remaining problem is that we are still showing new in various sample codes? If you can fix that quickly you can send it to me for review and we should be good as it is? Otherwise, I'd probably revert this PR for now.

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Sep 12, 2018

Contributor

Acutally new in samples have already been remove by #21539 . I was wondering if the flutter team had something in mind blocking this PR that I was not aware of.
What bother me is that this merge is so huge that any revert and/or all current PR will have conflict for merging.

Contributor

a14n commented Sep 12, 2018

Acutally new in samples have already been remove by #21539 . I was wondering if the flutter team had something in mind blocking this PR that I was not aware of.
What bother me is that this merge is so huge that any revert and/or all current PR will have conflict for merging.

@goderbauer

This comment has been minimized.

Show comment
Hide comment
@goderbauer

goderbauer Sep 12, 2018

Member

I am not aware of anything that would be blocking this. Let's leave this PR in. Here is my post-submit LGTM :)

LGTM

Member

goderbauer commented Sep 12, 2018

I am not aware of anything that would be blocking this. Let's leave this PR in. Here is my post-submit LGTM :)

LGTM

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Sep 12, 2018

Contributor

Thanks

Contributor

a14n commented Sep 12, 2018

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment