Skip to content

Fix poetenial issues in parser.py#1404

Merged
minggangw merged 2 commits intoRobotWebTools:developfrom
minggangw:fix-1403
Mar 3, 2026
Merged

Fix poetenial issues in parser.py#1404
minggangw merged 2 commits intoRobotWebTools:developfrom
minggangw:fix-1403

Conversation

@minggangw
Copy link
Member

@minggangw minggangw commented Mar 2, 2026

This PR tightens up the rosidl_parser/parser.py CLI wrapper used by the Node-side rosidl_parser.js helper by removing unused code and correcting the expected CLI argument count.

Changes:

  • Remove an unused rosidl_parser import.
  • Remove an unused intermediate msg_name variable.
  • Update the CLI argument count check to require 3 parameters after the script name (command, package, file path).

Fix: #1403

Copilot AI review requested due to automatic review settings March 2, 2026 09:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens up the rosidl_parser/parser.py CLI wrapper used by the Node-side rosidl_parser.js helper by removing unused code and correcting the expected CLI argument count.

Changes:

  • Remove an unused rosidl_parser import.
  • Remove an unused intermediate msg_name variable.
  • Update the CLI argument count check to require 3 parameters after the script name (command, package, file path).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 49 to 50
if len(sys.argv) < 4:
print('Wrong number of argments')
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

The CLI argument count check should likely reject both too-few and too-many arguments. Since this script expects exactly 3 parameters after the script name (command, packageName, filePath), consider checking len(sys.argv) != 4 and printing a usage string so callers can diagnose failures quickly. Also consider printing this error to stderr (like idl_parser.py does), otherwise Node’s execFile error handling may show an empty stderr.

Suggested change
if len(sys.argv) < 4:
print('Wrong number of argments')
if len(sys.argv) != 4:
print('Usage: {} <command> <packageName> <filePath>'.format(sys.argv[0]), file=sys.stderr)

Copilot uses AI. Check for mistakes.
if __name__ == '__main__':
if len(sys.argv) < 3:
if len(sys.argv) < 4:
print('Wrong number of argments')
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

Typo in error message: "argments" should be "arguments".

Suggested change
print('Wrong number of argments')
print('Wrong number of arguments')

Copilot uses AI. Check for mistakes.
@coveralls
Copy link

coveralls commented Mar 2, 2026

Coverage Status

coverage: 85.461% (+0.02%) from 85.441%
when pulling 45a406d on minggangw:fix-1403
into 1b3fdb9 on RobotWebTools:develop.

@minggangw minggangw merged commit 9070b0b into RobotWebTools:develop Mar 3, 2026
29 of 32 checks passed
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.

Potential issues in rosidl_parser/parser.py

3 participants