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

[2.19] Clarify library usage for no-name declarations update #4390

Merged
merged 21 commits into from
Dec 20, 2022

Conversation

MaryaBelanger
Copy link
Contributor

@MaryaBelanger MaryaBelanger commented Nov 18, 2022

fixes #4314

staged: https://lib-dir-docs.web.app

Changes:

I thought about completely removing the part of section in Effective Dart: Usage, in favor of the more general new section "Don't explicitly name libraries", because it's not even recommended to use part of at all anymore.

The new section could add a code example similar to the one for part of that more generally illustrates "Use URI strings to refer to libraries", and the linter rule could point to that section instead.

Thoughts? @kevmoo @pq etc.

@parlough
Copy link
Member

parlough commented Nov 18, 2022

Can you run git submodule update so you don't push an outdated site-shared submodule? Also consider git rebasing on main to avoid the complicated commit history.

@MaryaBelanger
Copy link
Contributor Author

@parlough Thanks for the tips, that kind of got away from me and wasn't sure what to do :/

@github-actions
Copy link

github-actions bot commented Nov 18, 2022

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

https://dart-dev--pr4390-library-directive-hasb6snu.web.app

(expires Mon, 28 Nov 2022 20:47:38 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: d851bc446d3c4d7394c5406c6f07255afc7075f3

@parlough parlough added review.copy Awaiting Copy Review review.tech Awaiting Technical Review labels Nov 18, 2022
@parlough parlough requested review from munificent and atsansone and removed request for munificent November 18, 2022 18:52
@parlough
Copy link
Member

@MaryaBelanger Let me know if you have any issues :)

@parlough parlough changed the title Clarify library usage for no-name declarations update [2.19] Clarify library usage for no-name declarations update Nov 18, 2022
Copy link
Member

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

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

Looks GREAT! Wait for at least one more approver, though!

Copy link
Member

@pq pq left a comment

Choose a reason for hiding this comment

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

💎

@pq
Copy link
Member

pq commented Nov 18, 2022

So awesome!

@MaryaBelanger
Copy link
Contributor Author

I'm actually going to change this to a draft because it shouldn't be merged until 2.19 releases, but thanks for the reviews everyone!

@MaryaBelanger MaryaBelanger marked this pull request as draft November 19, 2022 00:38
Copy link
Member

@munificent munificent left a comment

Choose a reason for hiding this comment

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

This is great!

src/_guides/language/effective-dart/documentation.md Outdated Show resolved Hide resolved
src/_guides/language/effective-dart/usage.md Outdated Show resolved Hide resolved
src/_guides/language/effective-dart/usage.md Outdated Show resolved Hide resolved
@MaryaBelanger
Copy link
Contributor Author

@munificent Thank you, incorporated all of that!

Going to mark as RFM

@MaryaBelanger MaryaBelanger removed the review.copy Awaiting Copy Review label Nov 21, 2022
@MaryaBelanger MaryaBelanger changed the base branch from v2.19 to main December 14, 2022 22:27
@MaryaBelanger MaryaBelanger changed the base branch from main to v2.19 December 14, 2022 22:28
@parlough parlough requested a review from atsansone December 16, 2022 08:53
@parlough parlough added review.copy Awaiting Copy Review and removed review.await-update Awaiting Updates after Edits labels Dec 16, 2022
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 super exciting to finally clean up our messaging around the library directive and have some proper suggestions. Nice job :D

I have a few comments and questions. Please push back on anything that doesn't make sense :)

src/_guides/libraries/create-library-packages.md Outdated Show resolved Hide resolved
src/_guides/language/language-tour.md Outdated Show resolved Hide resolved
src/_guides/language/language-tour.md Outdated Show resolved Hide resolved
src/_guides/language/effective-dart/style.md Outdated Show resolved Hide resolved
src/_guides/language/effective-dart/style.md Outdated Show resolved Hide resolved
src/_guides/language/effective-dart/usage.md Outdated Show resolved Hide resolved
src/_guides/language/effective-dart/style.md Show resolved Hide resolved
src/_guides/language/effective-dart/style.md Show resolved Hide resolved
@parlough parlough added review.await-update Awaiting Updates after Edits and removed review.copy Awaiting Copy Review labels Dec 19, 2022
@MaryaBelanger MaryaBelanger added st.RFM Ready to merge or land and removed review.await-update Awaiting Updates after Edits labels Dec 20, 2022
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 those adjustments. I responded to the unresolved discussion and left another potential suggestion.

src/_guides/language/language-tour.md Outdated Show resolved Hide resolved
src/_guides/libraries/create-library-packages.md Outdated Show resolved Hide resolved
src/_guides/language/language-tour.md Outdated Show resolved Hide resolved
@parlough parlough added review.await-update Awaiting Updates after Edits and removed st.RFM Ready to merge or land labels Dec 20, 2022
@MaryaBelanger MaryaBelanger added st.RFM Ready to merge or land and removed review.await-update Awaiting Updates after Edits labels Dec 20, 2022
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.

Love it!

src/_guides/language/language-tour.md Outdated Show resolved Hide resolved
Co-authored-by: Parker Lougheed <parlough@gmail.com>
@atsansone atsansone merged commit 458bcdb into v2.19 Dec 20, 2022
parlough added a commit that referenced this pull request Jan 23, 2023
fixes #4314

Changes:

- Added [code
sample](https://lib-dir-docs.web.app/guides/language/effective-dart/documentation#consider-writing-a-library-level-doc-comment)
to existing library-level doc comment section
- Added [Don't use library directives unless attaching doc comments or
annotations](https://lib-dir-docs.web.app/guides/language/effective-dart/usage#dont-use-library-directives-unless-attaching-doc-comments-or-annotations)
section
- Added [Don't explicitly name
libraries](https://lib-dir-docs.web.app/guides/language/effective-dart/usage#dont-explicitly-name-libraries)
section
- Reduced [`part
of`](https://lib-dir-docs.web.app/guides/language/effective-dart/usage#do-use-strings-in-part-of-directives)
section
- Removed / modified unnecessary notes about the library directive on
the [Creating
packages](https://lib-dir-docs.web.app/guides/libraries/create-library-packages)
page

I thought about completely removing the `part of` section in Effective
Dart: Usage, in favor of the more general new section "Don't explicitly
name libraries", because it's not even recommended to use `part of` at
all anymore.

The new section could add a code example similar to the one for `part
of` that more generally illustrates "Use URI strings to refer to
libraries", and the linter rule could point to that section instead.

Thoughts? @kevmoo @pq etc.

Co-authored-by: Parker Lougheed <parlough@gmail.com>
Co-authored-by: Kevin Moore <kevmoo@users.noreply.github.com>
parlough added a commit that referenced this pull request Jan 23, 2023
Fixes #4314

Changes:

- Added [code
sample](https://dart.dev/guides/language/effective-dart/documentation#consider-writing-a-library-level-doc-comment)
to existing library-level doc comment section
- Added [Don't use library directives unless attaching doc comments or
annotations](https://dart.dev/guides/language/effective-dart/usage#dont-use-library-directives-unless-attaching-doc-comments-or-annotations)
section
- Added [Don't explicitly name
libraries](https://dart.dev/guides/language/effective-dart/usage#dont-explicitly-name-libraries)
section
- Reduced [`part
of`](https://dart.dev/guides/language/effective-dart/usage#do-use-strings-in-part-of-directives)
section
- Removed / modified unnecessary notes about the library directive on
the [Creating
packages](https://dart.dev/guides/libraries/create-library-packages) page

Co-authored-by: Parker Lougheed <parlough@gmail.com>
Co-authored-by: Kevin Moore <kevmoo@users.noreply.github.com>
parlough added a commit that referenced this pull request Jan 24, 2023
Fixes #4314

Changes:

- Added [code
sample](https://dart.dev/guides/language/effective-dart/documentation#consider-writing-a-library-level-doc-comment)
to existing library-level doc comment section
- Added [Don't use library directives unless attaching doc comments or
annotations](https://dart.dev/guides/language/effective-dart/usage#dont-use-library-directives-unless-attaching-doc-comments-or-annotations)
section
- Added [Don't explicitly name
libraries](https://dart.dev/guides/language/effective-dart/usage#dont-explicitly-name-libraries)
section
- Reduced [`part
of`](https://dart.dev/guides/language/effective-dart/usage#do-use-strings-in-part-of-directives)
section
- Removed / modified unnecessary notes about the library directive on
the [Creating
packages](https://dart.dev/guides/libraries/create-library-packages) page

Co-authored-by: Parker Lougheed <parlough@gmail.com>
Co-authored-by: Kevin Moore <kevmoo@users.noreply.github.com>
parlough added a commit that referenced this pull request Jan 24, 2023
Fixes #4314

Changes:

- Added [code
sample](https://dart.dev/guides/language/effective-dart/documentation#consider-writing-a-library-level-doc-comment)
to existing library-level doc comment section
- Added [Don't use library directives unless attaching doc comments or
annotations](https://dart.dev/guides/language/effective-dart/usage#dont-use-library-directives-unless-attaching-doc-comments-or-annotations)
section
- Added [Don't explicitly name
libraries](https://dart.dev/guides/language/effective-dart/usage#dont-explicitly-name-libraries)
section
- Reduced [`part
of`](https://dart.dev/guides/language/effective-dart/usage#do-use-strings-in-part-of-directives)
section
- Removed / modified unnecessary notes about the library directive on
the [Creating
packages](https://dart.dev/guides/libraries/create-library-packages) page

Co-authored-by: Parker Lougheed <parlough@gmail.com>
Co-authored-by: Kevin Moore <kevmoo@users.noreply.github.com>
@parlough parlough deleted the library-directive branch January 24, 2023 17:12
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.19] Update documentation on Dart library directive to include non-name libraries
7 participants