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

Feature/flight structure #29

Closed
wants to merge 45 commits into from
Closed

Feature/flight structure #29

wants to merge 45 commits into from

Conversation

cjhr95
Copy link
Contributor

@cjhr95 cjhr95 commented Mar 4, 2022

Some basic repos for utility files and bare-bones states for state machine.

@mrouie
Copy link
Member

mrouie commented Mar 4, 2022

Make sure there's proper docstrings and type hinting everywhere, also I'd like you to put a single-line comment on all constants for their usage and purpose.

Also make sure all the xml files have comment headers at the beginning of the files after the xml version documenting the purpose and usage of the file.

flight/__init__.py Outdated Show resolved Hide resolved
flight/state_settings.py Show resolved Hide resolved
flight/state_settings.py Outdated Show resolved Hide resolved
flight/state_settings.py Show resolved Hide resolved
flight/states/pre_processing.py Show resolved Hide resolved
"""
raise Exception("Run not implemented for state")

async def _check_arm_or_arm(self, drone: System):
Copy link
Member

Choose a reason for hiding this comment

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

Missing return type hint

Copy link
Member

Choose a reason for hiding this comment

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

Still missing return type hint

flight/states/takeoff.py Show resolved Hide resolved


class Takeoff(State):
async def run(self, drone):
Copy link
Member

Choose a reason for hiding this comment

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

Missing docstring's params & returns and all function header type hints

flight/states/waypoints.py Show resolved Hide resolved


class Waypoints(State):
async def run(self, drone):
Copy link
Member

Choose a reason for hiding this comment

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

Missing function header type hints and function docstring

@mrouie mrouie added enhancement Advancement or augmentation of an already existing feature feature New feature to add flight Pertaining to the physical movement of the drone Infrastructure Core infrastructure-related task and removed enhancement Advancement or augmentation of an already existing feature labels Mar 7, 2022
@cjhr95 cjhr95 requested a review from mrouie March 9, 2022 17:34
@cjhr95 cjhr95 linked an issue Mar 9, 2022 that may be closed by this pull request
Copy link
Member

@mrouie mrouie left a comment

Choose a reason for hiding this comment

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

Make sure you run black on your code manually until I get the pre-commit set up

__run_description: Description for Competition
"""
def __init__(self, takeoff_bool: bool = False, num_waypoints: int = DEFAULT_WAYPOINTS,
title: str = DEFAULT_RUN_TITLE, description: str = DEFAULT_RUN_DESCRIPTION):
Copy link
Member

Choose a reason for hiding this comment

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

Missing type annotation for None return type

Member Variables:
None
"""
async def run(self, drone: System):
Copy link
Member

Choose a reason for hiding this comment

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

Missing None return type annotation

drone (System): The drone object; used for flight.
"""

def __init__(self, state_settings: StateSettings):
Copy link
Member

Choose a reason for hiding this comment

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

Still missing docstring & return type hint

logging.info('State "%s" has begun', self.name)
self.state_settings = state_settings

async def run(self, drone: System):
Copy link
Member

Choose a reason for hiding this comment

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

Still missing return type hint

"""
raise Exception("Run not implemented for state")

async def _check_arm_or_arm(self, drone: System):
Copy link
Member

Choose a reason for hiding this comment

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

Still missing return type hint

Comment on lines 5 to 8
f = open(filename, )
data_set = json.load(f)
# print(data_set)
f.close()
Copy link
Member

Choose a reason for hiding this comment

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

You should also definitely use a context manager whenever possible
It can avoid headaches when it comes to resource management

Comment on lines 19 to 22
f = open(filename, )
data_set = json.load(f)
# print(data_set)
f.close()
Copy link
Member

Choose a reason for hiding this comment

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

Same here, use a context manager

Comment on lines 24 to 29
stationary_Obs = []

for i in range(0, len(data_set["stationaryObstacles"])):
stationary_Obs.append(data_set["stationaryObstacles"][i])

return stationary_Obs
Copy link
Member

Choose a reason for hiding this comment

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

Also use a list comp here as well

return waypoint_Locs


def stationary_obstacle_parsing(filename: str):
Copy link
Member

Choose a reason for hiding this comment

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

please annotate your return type and initialized variables :)
also you're missing a docstring here too

Comment on lines 35 to 38
self.__simple_takeoff = takeoff_bool
self.__num_waypoints = num_waypoints
self.__run_title = title
self.__run_description = description
Copy link
Member

Choose a reason for hiding this comment

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

I know it may seem redundant, but since this is the first time we're referencing these variable names,
they should be typed as well

@cjhr95 cjhr95 requested a review from mrouie March 23, 2022 00:34
@cjhr95 cjhr95 closed this Apr 19, 2022
@cjhr95 cjhr95 deleted the feature/flight_structure branch April 19, 2022 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature to add flight Pertaining to the physical movement of the drone Infrastructure Core infrastructure-related task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants