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

[examples] Clean up visibility and paths #21401

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented May 7, 2024

The default visibility (for rules that aren't otherwise marked) shall always be private; making example code or models public should always be opt-in.

Align opt-in in to public visibility to exactly match things that are installed; if its not installed, then it shouldn't be used outside of Drake.

Avoid "subpackages" spelling for visibility; for clarity we should either make things fully public (installed) or else limit to an allow-list of specific other packages one by one.

Now that all examples code is suitably private, re-spell the imports to lose the "drake" part of the module namespace. The old spelling of imports is dispreferred and will not work with upcoming bzlmod (#20731).

Anzu CI has passed vs this PR.


This change is Reviewable

The default visibility (for rules that aren't otherwise marked) shall
always be private; making example code or models public should always
be opt-in.

Align opt-in in to public visibility to exactly match things that are
installed; if its not installed, then it shouldn't be used outside of
Drake.

Avoid "subpackages" spelling for visibility; for clarity we should
either make things fully public (installed) or else limit to an
allow-list of specific other packages one by one.

Now that all examples code is suitably private, re-spell the imports
to lose the "drake" part of the module namespace. The old spelling of
imports is dispreferred and will not work with upcoming bzlmod.
@jwnimmer-tri jwnimmer-tri added release notes: none This pull request should not be mentioned in the release notes priority: low status: single reviewer ok https://drake.mit.edu/reviewable.html labels May 7, 2024
@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri for both reviews, please.

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 46 of 46 files at r1, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignee rpoyner-tri(platform)

@rpoyner-tri rpoyner-tri merged commit 267951c into RobotLocomotion:master May 7, 2024
10 checks passed
@jwnimmer-tri jwnimmer-tri deleted the examples-visibility branch May 7, 2024 18:28
RussTedrake pushed a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024
The default visibility (for rules that aren't otherwise marked) shall
always be private; making example code or models public should always
be opt-in.

Align opt-in in to public visibility to exactly match things that are
installed; if its not installed, then it shouldn't be used outside of
Drake.

Avoid "subpackages" spelling for visibility; for clarity we should
either make things fully public (installed) or else limit to an
allow-list of specific other packages one by one.

Now that all examples code is suitably private, re-spell the imports
to lose the "drake" part of the module namespace. The old spelling of
imports is dispreferred and will not work with upcoming bzlmod.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: none This pull request should not be mentioned in the release notes status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants