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

Updated Language Evolution for chrono descending, plus clean up #4684

Merged
merged 9 commits into from
Apr 4, 2023

Conversation

atsansone
Copy link
Contributor

@atsansone atsansone commented Mar 17, 2023

This PR updates the Language Evolution page. I made some language updates, but re-sorted the versions as reverse chronological and added release dates and links to the release announcements.

@atsansone atsansone requested a review from MaryaBelanger March 17, 2023 20:09
@atsansone atsansone added the review.copy Awaiting Copy Review label Mar 17, 2023
@atsansone
Copy link
Contributor Author

@parlough parlough self-requested a review March 20, 2023 11:52
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks for preparing these changes!

I've gone through and left some issues, suggestions, and nits. Please feel free to push back or discuss anything.

One main thing is I think this doc really should only be focused on historical language changes and features, it's not a page for release notes. We should avoid adding SDK changes to be consistent and likely remove the few that exist already.

@parlough parlough added the review.await-update Awaiting Updates after Edits label Mar 20, 2023
Copy link
Contributor Author

@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.

@parlough : Merging my changes after this review.

@parlough parlough added review.tech Awaiting Technical Review and removed review.await-update Awaiting Updates after Edits labels Mar 25, 2023
@parlough parlough assigned parlough and unassigned atsansone Mar 25, 2023
@parlough parlough self-requested a review March 25, 2023 01:59
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks for making some adjustments and responding to my comments and questions!

Some of these review comments are responses to previous ones, so you'll need to look back at unresolved comments from the last review.

I'm also happy to discuss synchronously any of the concerns I raised if that works better for you as well.

site-shared Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This reverts to an older version of site-shared, please undo this change.

There's a few ways to do this. Two of these easiest might be:

  • git submodule foreach git pull origin main - Updates each submodule to track their latest commit on main. If you named dart-lang/site-www something besides origin, replace it with that.
  • Switch to a branch tracking the latest origin/main and run git submodule update there.

@parlough parlough removed their assignment Mar 28, 2023
@parlough parlough added the review.await-update Awaiting Updates after Edits label Mar 28, 2023
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

It's getting super close. Thanks for all of those changes.

A few last comments and answers to questions:

Dart 2.6 introduced a
[breaking change (dart-lang/sdk#37985)](https://github.com/dart-lang/sdk/issues/37985).
Inference changes when using Null values in a `FutureOr` context.
Constraints of the forms similar to `Null <:FutureOr` now yield `Null`
Copy link
Member

Choose a reason for hiding this comment

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

The <: should be outside of the code.

Suggested change
Constraints of the forms similar to `Null <:FutureOr` now yield `Null`
Constraints of forms similar to `Null` <: `FutureOr` now yield `Null`

Also we might want to avoid the symbol completely. It's something readers may not be familiar with. Perhaps something like?

Suggested change
Constraints of the forms similar to `Null <:FutureOr` now yield `Null`
Constraints of the form where `Null` is a subtype of `FutureOr` now yield `Null`

@parlough parlough force-pushed the revise-evo branch 2 times, most recently from e8c474f to 0a835e8 Compare March 30, 2023 06:25
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

I had to rebase these changes to fix some issues with the PR and pushed some minor changes. There's two last changes that need to be addressed before merging but I want you to take a look.

Otherwise, looks good to me. Thanks for doing this and all of the discussion. I think the page has got to a really great spot and has a clearer vision now.

Optionally get a second review from someone after making those fixes, but since it's mostly reorganized and rewritten existing content, it should be fine.

Comment on lines 236 to 237
Constraints of the forms similar to `Null` serves as a subtype of `FutureOr`
now yield `Null` as the solution for `T`.
Copy link
Member

Choose a reason for hiding this comment

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

I think the "Constraints of the forms similar to" portion is pretty confusing, can we simplify it? I also didn't notice the missing generic on FutureOr<T>. That's important for knowing what the following T is referring to.

Suggested change
Constraints of the forms similar to `Null` serves as a subtype of `FutureOr`
now yield `Null` as the solution for `T`.
Constraints where `Null` serves as a subtype of `FutureOr<T>`
now yield `Null` as the solution for `T`.

of the SDK constraint in the `pubspec.yaml` file.

**For example:** The following entry in a `pubspec.yaml` file
indicates that this package uses the Dart 2.18 language version or later.
Copy link
Member

Choose a reason for hiding this comment

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

This indicates it uses the 2.18 language version by default, not later. It can't use later language versions or else that would break the constraint.

Suggested change
indicates that this package uses the Dart 2.18 language version or later.
indicates that this package uses the Dart 2.18 language version by default.

@parlough parlough added st.RFM.% Ready to Merge but has suggestions review.await-update Awaiting Updates after Edits and removed review.await-update Awaiting Updates after Edits review.tech Awaiting Technical Review labels Mar 30, 2023
@atsansone atsansone added st.RFM Ready to merge or land and removed review.copy Awaiting Copy Review review.await-update Awaiting Updates after Edits st.RFM.% Ready to Merge but has suggestions labels Apr 4, 2023
@atsansone atsansone merged commit 012d940 into dart-lang:main Apr 4, 2023
rmacnak-google pushed a commit to rmacnak-google/site-www that referenced this pull request Sep 5, 2023
…-lang#4684)

This PR updates the Language Evolution page. I made some language
updates, but re-sorted the versions as reverse chronological and added
release dates and links to the release announcements.

---------

Co-authored-by: Parker Lougheed <parlough@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
st.RFM Ready to merge or land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants