First challenge #7
Conversation
There was a problem hiding this comment.
Submission / Git Practice:
- The title of the pull request does not follow the formula provided in the challenge instructions
- The branch name is well done
- There is no description for the pull request, you typically always want to briefly summarize what changes are in the pull request
- The commit message does not indicate what changes were made in the commit, commit messages should be concise, descriptive comments about what you changed/added in the commit.
Code:
Overall well done, the comments are primarily style and other nit picky things like spelling. These are usually a pretty common thing to have noticed in pull requests. This challenge was much more focused on the workflow then the actual code,
Few things tp note:
- navigate() can only be called once and then will block in an infinite loop of reading the gps sensor
- pass is left in the code unnecessarily
- Unused variable moveX
- The navigate method will only update one Axis at a time as it returns after effecting the desired change, it may have been more efficient to make changes for each axis in a single call to navigate
|
|
||
|
|
There was a problem hiding this comment.
AS per PEP8 - all methods are surrounded only by a single line of whitespace
| def update_current_position(self): | ||
| """ Updates the current position of the TBM """ | ||
| self.current_position = self.GPS.getPos() | ||
| pass |
There was a problem hiding this comment.
This line serves no purpose right now, all uses of pass should be removed when the method is implemented.
There was a problem hiding this comment.
It just serves as a placeholder
|
|
||
| success = true | ||
|
|
||
| # Assume all move commands with distance esentially finish instintaniously and |
There was a problem hiding this comment.
*essentially, *instantaneously
| @@ -32,27 +32,27 @@ class will interface with our onboard GPS sensor to get thecurrent position, and | |||
| | | |||
| | | |||
| below target | |||
|
|
|||
There was a problem hiding this comment.
Is there any reason there is a lot of lines removed and then added - just wondering if your ide is telling you to indent with tabs or something
There was a problem hiding this comment.
@Akashf I'm assuming you ran a linter or something over the code and it picked up some trailing whitespace. For future reference, if you make any linting changes, try to put them in their own commit. Makes it easier for the reviewer to actually see your code changes.
| if moveY > 0: | ||
| success = self.steering.move_right(moveY) | ||
| if !success: | ||
| print("Move right did not execute properly") # print used as a substitute for any place that we'd want to print to |
There was a problem hiding this comment.
The comment explaining print as a placeholder for a log is good but could have been placed above the call to print to adhere to the 80 char line length limit
| while self.current_position != self.desired_position: | ||
| self.update_current_position() |
There was a problem hiding this comment.
I think this will be an infinite while loop that just reads the GPS, this would block the application when navigate is called.
| # Assume it only has to move forward in the X direction. | ||
| if self.current_position != self.desired_position: |
There was a problem hiding this comment.
I understand that this code is only reached when Z and Y have both been adjusted, but it would make it easier to read if you specified that you are checking x here. Also, moveX is an unused variable
|
|
||
| # Assume it only has to move forward in the X direction. | ||
| if self.current_position != self.desired_position: | ||
| success = self.steering.move_forward() |
There was a problem hiding this comment.
move_forward probably should have an argument for how far to move.
No description provided.