-
Notifications
You must be signed in to change notification settings - Fork 226
Update enhanced-parts/feature-specification.md #4520
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
base: main
Are you sure you want to change the base?
Conversation
Maybe "assigned" should be "reviewer"? |
They should! And now they are. |
Thanks! I won't get around to it today, but tomorrow should work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
I added a bunch of comments. I'd prefer if we're slightly more careful about the characterization of the current rules (it's not the purpose of this document to change the existing rules). Also, I find the use of the phrase 'combined import scope' confusing, and I'd prefer to have a characterization which is consistently based on a scope tree (where edges go from each scope to its enclosing scope). Also, a couple of questions arise when it comes to shadowing and prefixes.
I'd like to know whether there is anything in this specification which is intended to contradict the scoping structure which is shown at the top of #4082. Otherwise, that might serve as a checklist to consider at each step when reading the rules in this document.
Nothing is intended to disagree with the diagram of #4082. The library introduces:
Each file introduces:
A file introduces one scope per per namespace that it introduces, with the following parent scopes:
A file also has a top-level declaration scope whose namespace is the library's declaration scope, and its parent scope is the file's prefix scope. The text uses both "prefix scope" and "combined import scope" about the same scope, usually depending on whether the text focuses on the namespace or the entire chain. That's probably confusing, it should use the same name every time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't follow all of the details of the scoping rules but it seems like you and Erik are hashing that out fine. :) If you want me to dig in, let me know and I'll go through it in detail.
Otherwise, LGTM!
library to keep its deprecated API, and its necessary imports, separate from the | ||
rest, so that it can all be removed as a single operation, and then marking all | ||
that API as deprecated with one annotation._ | ||
library to keep its deprecated API, and that APIs necessary imports, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "APIs" -> "API's".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick search suggests that "most style guides" recommend not using an apostrophe with a plural initialism.
The University of Oxford Style Guide agrees.
Remove configurable part directives. General tweaks and rephrasings, and some typos. Mention extension conflicts. (You can break sub-parts by importing an extension that causes a conflict. You can protect a part against that by using explicit extension applications.)
5f0f885
to
008b2a0
Compare
Addressed comments. Will land on Monday if no complaints. |
Remove configurable part directives.
General tweaks and phrasing fixes, and some typos fixed.
Mention extension conflicts.
(You can break sub-parts by importing an extension that causes a conflict.
You can protect a part against that by using explicit extension applications.
Most people won't ever notice, extension conflicts are rare.)