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

README: Minor stylistic changes, add documentation links #243

Merged

Conversation

EricCousineau-TRI
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI commented Apr 15, 2020

  • Discourages (or rather does not encourage) use of sudo make [un]install - it's a Bad Idea™
  • Updates Markdown formatting
  • Add explicit links to sdf_tutorials and sdformat.org

@EricCousineau-TRI
Copy link
Collaborator Author

@EricCousineau-TRI EricCousineau-TRI changed the title README: Minor stylistic changes README: Minor stylistic changes, add documentation links Apr 15, 2020
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@EricCousineau-TRI
Copy link
Collaborator Author

What is the review policy for this repo? When do I squash? (I really like to make sure my commits are curated, esp. now that BitBucket is no longer being used.)

@chapulina
Copy link
Contributor

What is the review policy for this repo? When do I squash?

We're still figuring these things out internally. Suggestions are very welcome.

@EricCousineau-TRI
Copy link
Collaborator Author

I like the ROS2 guidelines (when using plain vanilla GitHub review), which is to always have fast-forward pushes for review, but then curate commits at the end, possibly after approval.
https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#pull-requests

Would that work for you?

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Apr 16, 2020

Ooh, also, @scpeters and @azeey and I may try out Reviewable (example draft PR here: gazebosim/sdf_tutorials#3). This is perhaps not actionable now, but I could let you know how that goes for us?

@chapulina
Copy link
Contributor

always have fast-forward pushes for review, but then curate commits at the end, possibly after approval.

That sounds reasonable to me, we'll discuss this as a team next week.

Reviewable

We're also planning to discuss this. My main concern is the recommendation to not mix the GitHub and Reviewable interfaces. Do you have any experience mixing both?

The entire team at Open Robotics should be able to open and review SDFormat PRs. We should also strive to have a consistent workflow for the entire Ignition family. So it sounds like we either use Reviewable for all projects or none.

@EricCousineau-TRI
Copy link
Collaborator Author

That sounds reasonable to me, we'll discuss this as a team next week.

Sounds good. For this PR, I will wait for Approval, then curate my commits.

[...] Do you have any experience mixing both?

We just tried it out here: gazebosim/sdf_tutorials#3 (review)
Basically, it works from GitHub to Reviewable (i.e. I see a new discussion in Reviewable, started by the GitHub item). However, once I responded in Reviewable, it did not go back to the GitHub discussion, as we've noted here for Drake:
https://drake.mit.edu/reviewable.html#github-integration
I think that can easily be solved by just referencing it.

The main issue we've had is mixing the request of review between GitHub (assigning reviewers in the sidebar) vs Reviewable (+@username). It does not always recognize GitHub assignment, so we generally stick to Reviewable assignments. [*]

So overall, the mixing between GitHub and Reviewable is not the most convenient, but may be workable.
Additionally, once we've become accustomed to Reviewable, it's wayyy easier to know that everything has been addressed in the review (and really easy to get the most de-cluttered view of it), with minimal effort of going back and looking through conversation history or what not in GitHub. The excessive client-side stuff is a bit annoying, but eh, overall I say worth it.

Understandably, this may not work for y'all, but I would recommend people taking it out for a test run. It's easy to point Reviewable to a PR to try it out on.


[*] We also have the following completion script, so it's unclear if this is due to our custom completion script (meant to handle labels and such):
https://github.com/RobotLocomotion/drake-ci/wiki/reviewable.io-review-completion-condition

@EricCousineau-TRI
Copy link
Collaborator Author

Are there any action items remaining here?

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
@EricCousineau-TRI
Copy link
Collaborator Author

Squashed to two curated commits

@EricCousineau-TRI
Copy link
Collaborator Author

And thanks!

@scpeters scpeters merged commit 8d644b9 into gazebosim:master Jun 4, 2020
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