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.

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

Deprecate describeEnum + type stricter EnumProperty #9003

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Deprecate describeEnum + type stricter EnumProperty #9003

merged 2 commits into from
Aug 3, 2023

Conversation

bernaferrari
Copy link
Contributor

Sequel to #8571 and flutter/flutter#125016. This is super small, but broke a test that copied a Flutter file (and was not modified when Flutter got updated).

@parlough parlough added the review.copy Awaiting Copy Review label Jul 9, 2023
sfshaza2
sfshaza2 requested changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

I have some rather large changes. Also, the previous "Deprecate describeEnum" PR has some of the same issues.

src/release/breaking-changes/enum-property-type-stricter.md Outdated
## Summary

The class `EnumProperty` was modified to extend `<T extends Enum?>` instead of
`<T>`. Existing uses of `EnumProperty<NotAnEnum>` should use
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`<T>`. Existing uses of `EnumProperty<NotAnEnum>` should use
`<T>`. Previous uses of `EnumProperty<NotAnEnum>` should use

src/release/breaking-changes/enum-property-type-stricter.md Outdated
Comment on lines 14 to 16
Dart 2.17 introduced enhanced enums. With them, Enum became a type.
Before that, it was common to use classes as enums, and `EnumProperty`
was used to represent them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Dart 2.17 introduced enhanced enums. With them, Enum became a type.
Before that, it was common to use classes as enums, and `EnumProperty`
was used to represent them.
Dart 2.17 introduced [enhanced enums][], which added `Enum` as a type.
Before that, enum classes were often defined as an `EnumProperty`.
[enhanced enums]: {{site.dart-site}}/language/enums#declaring-enhanced-enums

src/release/breaking-changes/enum-property-type-stricter.md Outdated Show resolved Hide resolved
src/release/breaking-changes/enum-property-type-stricter.md Outdated Show resolved Hide resolved
src/release/breaking-changes/enum-property-type-stricter.md Outdated Show resolved Hide resolved
src/release/breaking-changes/enum-property-type-stricter.md Outdated Show resolved Hide resolved
src/release/breaking-changes/enum-property-type-stricter.md Outdated
@@ -0,0 +1,72 @@
---
title: Migration guide for EnumProperty
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
title: Migration guide for EnumProperty
title: Update EnumProperty to be type strict

src/release/breaking-changes/enum-property-type-stricter.md Outdated Show resolved Hide resolved
src/release/breaking-changes/index.md Outdated Show resolved Hide resolved
src/release/breaking-changes/index.md Outdated Show resolved Hide resolved
@sfshaza2 sfshaza2 added review.await-update Awaiting Updates after Edits and removed review.copy Awaiting Copy Review labels Jul 10, 2023
@bernaferrari bernaferrari changed the title Make EnumProperty type stricter Depracte describeEnum + type stricter EnumProperty Jul 10, 2023
@bernaferrari
Copy link
Contributor Author

I merged the older describeEnum document with this method (so it is a single document). Tell me how you feel.

@bernaferrari bernaferrari changed the title Depracte describeEnum + type stricter EnumProperty Deprecate describeEnum + type stricter EnumProperty Jul 10, 2023
@bernaferrari
Copy link
Contributor Author

Friendly bump.

@bernaferrari
Copy link
Contributor Author

cc @sfshaza2

@parlough parlough added review.copy Awaiting Copy Review and removed review.await-update Awaiting Updates after Edits labels Jul 26, 2023
sfshaza2
sfshaza2 requested changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

Sorry I missed your updates before. This looks much better, but there are a few little things, still.

src/release/breaking-changes/describe-enum.md Outdated Show resolved Hide resolved
src/release/breaking-changes/describe-enum.md Outdated Show resolved Hide resolved
src/release/breaking-changes/describe-enum.md
Comment on lines 93 to -75

[☂️ Cleanup SemanticsFlag and SemanticsAction issue]: {{site.repo.flutter}}/issues/123346
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you delete this line? It should be:

- [Cleanup SemanticsFlag and SemanticsAction issue][cleanup-issue]

And then [cleanup-issue] is defined below. It allows a shortcut to that loooong title.

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 don't understand but I think I did this now. I put the long title reference in the related issues, and in the master link it links to the shorter title.

@bernaferrari
Fix?
d1629b4 
Fix link.

Fix link.

Try to fix lint issue

;

Improve docs.

Refactor everything.

Change file.

EnumProperty
sfshaza2
sfshaza2 approved these changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

lgtm

@sfshaza2 sfshaza2 removed the review.copy Awaiting Copy Review label Aug 3, 2023
@bernaferrari
Copy link
Contributor Author

bernaferrari commented Aug 3, 2023 •
edited
Loading

Is the failing check intended (will be auto fixed once the original thing is merged)? It seems to be 404 itself..

@bernaferrari
Copy link
Contributor Author

bernaferrari commented Aug 3, 2023 •
edited
Loading

(do I still need the review from @parlough ? It's been a while I don't break anything, I forgot how the process is).

@sfshaza2 sfshaza2 merged commit 6faccb4 into flutter:main Aug 3, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfshaza2 sfshaza2 sfshaza2 approved these changes

@atsansone atsansone atsansone approved these changes

@parlough parlough Awaiting requested review from parlough parlough is a code owner

Assignees

@sfshaza2 sfshaza2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bernaferrari @atsansone @sfshaza2 @parlough