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

Refactor populateVelocities #14

Merged
merged 2 commits into from
Feb 24, 2014
Merged

Refactor populateVelocities #14

merged 2 commits into from
Feb 24, 2014

Conversation

po1
Copy link
Contributor

@po1 po1 commented Feb 18, 2014

Also added documentation and removed the node greeting log statement ( 😿 )

@po1
Copy link
Contributor Author

po1 commented Feb 18, 2014

@adolfo-rt this pull request would be a good place to add a unit test (#8) :)

po1 referenced this pull request Feb 19, 2014
Revert intrusive changes to main function.
ce5d11d 
- Hide async spinner requirements of the approach planner to its implementation.

- Don't swallow unexpected exceptions. Let the message show on program
  termination.

- Bonus: Reduce header dependencies of approach planner.
@po1
Copy link
Contributor Author

po1 commented Feb 21, 2014

@adolfo-rt please also squash the commits to avoid needlessly cluttering the commit log

@adolfo-rt
Copy link
Contributor

Done

@po1 po1 assigned adolfo-rt and unassigned adolfo-rt Feb 21, 2014
@po1
Copy link
Contributor Author

po1 commented Feb 24, 2014

Looks like some other refactoring broke this.
Also, @adolfo-rt, will you have time to implement a unit test for populateVelocities this week?

@adolfo-rt
Copy link
Contributor

Not that I don't want to do it, but there is no prevision for writing tests in the coming weeks. I was hoping to do this in April or so. My current set of patches to play_motion will need at least a whole week of test writing, maybe more. The populateVelocities part could take 1 - 1.5 days.

What broke exactly?

@po1
Copy link
Contributor Author

po1 commented Feb 24, 2014

I'll rebase this PR and fix the conflicts. It is just about code moving around and patches not applying properly.
I will take back the initial suggestion that a unit test (#8) should be done here and merge it early, as I want to make the last release before the merge with the planning branch.

Adolfo Rodriguez Tsouroukdissian added 2 commits February 24, 2014 11:23
Refactor populateVelocities. Document it.
b66ed06
Revert intrusive changes to main function.
8a787f0 
- Don't swallow unexpected exceptions. Let the message show on program
  termination.

- Hide async spinner requirements of the approach planner to its implementation.
@adolfo-rt
Copy link
Contributor

I will take back the initial suggestion that a unit test (#8) should be done here and merge it early, as I want to make the last release before the merge with the planning branch.

That merge will probably be a painful one.

@po1
Copy link
Contributor Author

po1 commented Feb 24, 2014

Rebased, built, passed the unit tests. Merging.

po1 added a commit that referenced this pull request Feb 24, 2014
@po1
Merge pull request #14 from pal-robotics/refactor-popuvel
070c639 
Refactor populateVelocities
@po1 po1 merged commit 070c639 into hydro-devel Feb 24, 2014
@po1 po1 deleted the refactor-popuvel branch February 24, 2014 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@po1 @adolfo-rt