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

Announce a breaking change of added dispose(). #9397

Merged
merged 21 commits into from
Sep 18, 2023
Merged

Conversation

polina-c
Copy link
Contributor

@polina-c polina-c commented Sep 14, 2023

Discussion: https://docs.google.com/document/d/1ec5djPP14b1u93O9fHrJJf3gbONay1nV4sBV2-XXOIw/edit?resourcekey=0-CFEu4HWAW8yP0knhoVfKxQ

This announce is added as already released, because we are adding dispose already long time and some of the changes may be already released: flutter/devtools#3951

Tech writers, please do not merge this before tech sign off. @goderbauer , can you review please?

@polina-c polina-c changed the title Create dispose.md Announce a breaking change. Sep 14, 2023
@github-actions
Copy link

github-actions bot commented Sep 14, 2023

Visit the preview URL for this PR (updated for commit d0cd593):

https://flutter-docs-prod--pr9397-polina-c-dispose-h8iaiomp.web.app

(expires Mon, 25 Sep 2023 18:16:22 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: d5ba327eec813901cac8396c4f458b02288624ab

@polina-c polina-c changed the title Announce a breaking change. Announce a breaking change of added dispose(). Sep 14, 2023
@polina-c polina-c marked this pull request as ready for review September 14, 2023 22:05
@polina-c polina-c requested a review from goderbauer September 14, 2023 22:08
Copy link
Contributor

@atsansone atsansone left a comment

Choose a reason for hiding this comment

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

@polina-c : A few suggestions which you can accept or not. LGTM % your review of those suggestions.

src/release/breaking-changes/dispose.md Outdated Show resolved Hide resolved
src/release/breaking-changes/dispose.md Outdated Show resolved Hide resolved
Comment on lines 25 to 27
If you got error like `Once you have called dispose() on a <class name>, it can no longer be used.`,
and the error is raised for your code,
update the code to call `dispose()' only in cases when your code created the object.
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Write as shorter sentences per GSG.

Suggested change
If you got error like `Once you have called dispose() on a <class name>, it can no longer be used.`,
and the error is raised for your code,
update the code to call `dispose()' only in cases when your code created the object.
If the following error was raised for your code,
update the code to call `dispose()' only in cases when your code created the object.
```nocode
Once you have called dispose() on a <class name>, it can no longer be used.
```

Copy link
Contributor

@sfshaza2 sfshaza2 Sep 15, 2023

Choose a reason for hiding this comment

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

Please fix the grammar:
If you encounter the following error, then check for
that condition:

Once you have called dispose() on a <class name>, it can no longer be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shams, in most cases, the code should not be removed, but condition should be added. So, it should be fixed, not removed.

@atsansone atsansone added the st.RFM.% Ready to merge or land with minor changes. No further review needed. label Sep 15, 2023
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.

A few grammatical improvements.

src/release/breaking-changes/dispose.md Outdated Show resolved Hide resolved
src/release/breaking-changes/dispose.md Outdated Show resolved Hide resolved
src/release/breaking-changes/dispose.md Outdated Show resolved Hide resolved
src/release/breaking-changes/dispose.md Outdated Show resolved Hide resolved
Comment on lines 25 to 27
If you got error like `Once you have called dispose() on a <class name>, it can no longer be used.`,
and the error is raised for your code,
update the code to call `dispose()' only in cases when your code created the object.
Copy link
Contributor

@sfshaza2 sfshaza2 Sep 15, 2023

Choose a reason for hiding this comment

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

Please fix the grammar:
If you encounter the following error, then check for
that condition:

Once you have called dispose() on a <class name>, it can no longer be used.

src/release/breaking-changes/dispose.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
Copy link
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

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

LGTM, but @goderbauer should review before merge

src/release/breaking-changes/dispose.md Outdated Show resolved Hide resolved
src/release/breaking-changes/index.md Outdated Show resolved Hide resolved
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

src/release/breaking-changes/dispose.md Show resolved Hide resolved
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

@polina-c polina-c merged commit 6bb94a4 into main Sep 18, 2023
@polina-c polina-c deleted the polina-c-dispose branch September 18, 2023 18:55
atsansone pushed a commit to atsansone/website that referenced this pull request Sep 19, 2023
@parlough parlough removed the st.RFM.% Ready to merge or land with minor changes. No further review needed. label Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants