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

Add file_sync_test and start to re-home common files #307

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

jwnimmer-tri
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri commented Sep 19, 2024

The idea here is that users should always be able to copy just one subdirectory and end up with everything they need. To the extent that we have important files outside of the drake_... subdirectories, the examples will be broken.

Of course that often will mean duplicating content across the sub-directories, which leads to a maintenance hazard. We can solve that by using this new test program to cross-check that the copies are in sync.

This just moves one file as a starter pack, to bootstrap the idea. Over time, we should make further adjustments and copies so that everything inside the drake_... subdirs is 100% standalone.

For now, maintainers will need to run this by hand. Soon we plan to have #308 to add it to CI.


This change is Reviewable

@jwnimmer-tri
Copy link
Contributor Author

+@BetsyMcPhail for feature review or delegation, please.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Testing locally on my system, I confirmed that changing a .clang-format file results in an error message.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @jwnimmer-tri)


private/test/file_sync_test.py line 37 at r1 (raw file):

    # For readability, enforce that the lists are sorted.
    first_name = Path(paths[0]).name
    prologue = f"The {index+1}th list of files (containing {first_name})"

Nit: As there is only one file copied, this results in the error message "ERROR: The 1th list of files (containing .clang-format) do not all match"

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail 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: 1 unresolved discussion, platform LGTM from [betsymcphail] (waiting on @jwnimmer-tri)

Copy link
Contributor Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

+@rpoyner-tri for platform review, please.

Reviewable status: all discussions resolved, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @rpoyner-tri)


private/test/file_sync_test.py line 37 at r1 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

Nit: As there is only one file copied, this results in the error message "ERROR: The 1th list of files (containing .clang-format) do not all match"

Right. In the future we will have many more groups of copied files, and I don't know of a good way to have the f-string use proper grammar easily.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewable status: all discussions resolved, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @rpoyner-tri)


private/test/file_sync_test.py line 37 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Right. In the future we will have many more groups of copied files, and I don't know of a good way to have the f-string use proper grammar easily.

Works for me, the important part is the failure.

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 9 of 9 files at r1, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @jwnimmer-tri)

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, platform LGTM from [betsymcphail, rpoyner-tri] (waiting on @jwnimmer-tri)

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.

Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail, rpoyner-tri] (waiting on @jwnimmer-tri)

a discussion (no related file):
Huh. Merge button is there for me, but doesn't work.


@jwnimmer-tri jwnimmer-tri merged commit 8787710 into RobotLocomotion:main Sep 20, 2024
5 of 6 checks passed
@jwnimmer-tri jwnimmer-tri deleted the file-checker branch September 20, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants