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

Document ModuleData and improve names #80425

Merged
merged 2 commits into from
Jan 7, 2021
Merged

Document ModuleData and improve names #80425

merged 2 commits into from
Jan 7, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Dec 28, 2020 •
edited
Loading

  • Document ModuleData
  • Rename ModuleData.normal_ancestor_id to nearest_parent_mod
  • Rename Resolver::nearest_mod_parent to nearest_parent_mod

cc https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/mentoring/near/221029702

r? @petrochenkov

@camelid camelid added A-resolve Area: Name/path resolution done by `rustc_resolve` specifically T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels Dec 28, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2020
@camelid
Copy link
Member Author

camelid commented Dec 28, 2020 •
edited
Loading

Rename ModuleData.normal_ancestor_id to closest_parent_mod

There seems to be a function called nearest_mod_parent in resolve; its docs seem to suggest that the result from it would be equivalent to ModuleData.normal_ancestor_id?

Also, perhaps nearest_mod might be a better name?

@camelid camelid force-pushed the resolve-moduledata-docs branch from a534373 to ba4c1de Compare December 28, 2020 01:01
@petrochenkov
Copy link
Contributor

The normal_ancestor_id naming is ancient, and I agree that it's very confusing.
closest_parent_mod is a good replacement, nearest_parent_mod might be better, "nearest" seems to be a commonly used work in graph theory (I think it's important to keep "parent").

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 28, 2020
@camelid
Copy link
Member Author

camelid commented Dec 28, 2020

closest_parent_mod is a good replacement, nearest_parent_mod might be better, "nearest" seems to be a commonly used work in graph theory (I think it's important to keep "parent").

Will it be confusing if there's ModuleData.nearest_parent_mod and Resolver::nearest_mod_parent()?

@petrochenkov
Copy link
Contributor

nearest_mod_parent can be renamed to nearest_parent_mod too for consistency.

@camelid camelid changed the title Document ModuleData Document ModuleData and improve names Jan 6, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2021
@camelid camelid force-pushed the resolve-moduledata-docs branch from 13f427e to 8edcdb9 Compare January 6, 2021 01:12
@petrochenkov
Copy link
Contributor

r=me after squashing commits.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2021
@camelid camelid force-pushed the resolve-moduledata-docs branch from dce53a7 to f0598e5 Compare January 6, 2021 20:40
@camelid
Copy link
Member Author

camelid commented Jan 6, 2021

Rebased.

@camelid
Document ModuleData
0693198 
* Convert comments on fields to doc comments so they're visible in API
  docs
* Add new documentation
* Get rid of "normal module" terminology
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 6, 2021

📌 Commit 5c86c223ee890dbdfd136e8eb9f0a404bc9a88bf has been approved by petrochenkov

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 6, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 6, 2021
@camelid
Copy link
Member Author

camelid commented Jan 6, 2021

Oh, I was about to push up squashed commits. Do you still want me to do that?

@camelid
Rename to nearest_parent_mod
ff75da8 
* Rename `ModuleData.normal_ancestor_id` to `nearest_parent_mod`

`normal_ancestor_id` is a very confusing name if you don't already
understand what it means. Adding docs helps, but using a clearer and
more obvious name is also important.

* Rename `Resolver::nearest_mod_parent` to `nearest_parent_mod`

* Add more docs
@petrochenkov
Copy link
Contributor

Yes, please push.
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 6, 2021
@camelid camelid force-pushed the resolve-moduledata-docs branch from 5c86c22 to ff75da8 Compare January 6, 2021 20:56
@camelid
Copy link
Member Author

camelid commented Jan 6, 2021

Squashed.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 6, 2021

📌 Commit ff75da8 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2021
@bors
Copy link
Contributor

bors commented Jan 7, 2021

⌛ Testing commit ff75da8 with merge 5b3d524...

@bors
Copy link
Contributor

bors commented Jan 7, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 5b3d524 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 7, 2021
@bors bors merged commit 5b3d524 into rust-lang:master Jan 7, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 7, 2021
This was referenced Jan 7, 2021
Closed
Merged
@camelid camelid deleted the resolve-moduledata-docs branch January 7, 2021 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools A-resolve Area: Name/path resolution done by `rustc_resolve` specifically merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.51.0
Development

Successfully merging this pull request may close these issues.
6 participants
@camelid @petrochenkov @rust-log-analyzer @bors @rust-highfive @rustbot