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

Creates Arch.yml using rosnode list #80

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Creates Arch.yml using rosnode list #80

wants to merge 10 commits into from

Conversation

mvkashyap
Copy link
Collaborator

Changes to be committed:
new file: src/rosdiscover/Issue74.py

 Changes to be committed:
	new file:   src/rosdiscover/Issue74.py
src/rosdiscover/Issue74.py Outdated Show resolved Hide resolved
image = 'therobotcooperative/turtlebot3'
sources = ['/opt/ros/kinetic/setup.bash', '/ros_ws/devel/setup.bash']
environment = {'TURTLEBOT3_MODEL': 'burger'}
with rsw.launch(image, sources, environment=environment) as system:
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's refactor this into a function

src/rosdiscover/Issue74.py Outdated Show resolved Hide resolved
service_to_format[service_name] = service.format.name


nodeSummaryDict = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would refactor this a separate function too. That takes a state and returns NodeSummaryDict.

I think also, for this, you could try using mypy typing like we do in the other parts of the code.

	modified:   src/rosdiscover/cli.py
@mvkashyap
Copy link
Collaborator Author

I know that the checks will fail... I just needed to push whatever work I had so that I could pull it from another computer.

@ChrisTimperley
Copy link
Member

I know that the checks will fail... I just needed to push whatever work I had so that I could pull it from another computer.

That's perfectly fine! 🙂 You should feel free to make more, smaller commits, rather than worrying about making fewer, larger ones. Also, don't forget to add short but descriptive messages to your commits.

	deleted:    src/rosdiscover/Issue74.py
	modified:   src/rosdiscover/cli.py
	modified:   src/rosdiscover/models/gazebo_gui.py
	modified:   src/rosdiscover/models/joy.py
	modified:   src/rosdiscover/models/twist_mux.py

Added 'dynamic' feature to cli.py that dynamically analyzes nodes.
src/rosdiscover/cli.py Outdated Show resolved Hide resolved
package=package,
args={'gui': 'false'})

time.sleep(30)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add this as an optional parameter, but default it to 30.

environment = data['environment']
node_names, state, topic_to_type, service_to_format = get_info(image, sources, environment,
args.launchfile, args.package)
nodeSummaryDict = create_dict(node_names, state, topic_to_type, service_to_format)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the following code can be done more simply, like we discussed yesterday

node_names, state, topic_to_type, service_to_format = get_info(image, sources, environment,
args.launchfile, args.package, sleep_time)
node_summary_dict = create_dict(node_names, state, topic_to_type, service_to_format)
f.write(json.dumps(node_summary_dict, indent=4, separators=(". ", " = ")))
Copy link
Member

Choose a reason for hiding this comment

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

The separators argument here should be dropped to avoid producing bad JSON.

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