Quality of Life Fixes #1123
Quality of Life Fixes #1123
Conversation
|
Of note, two tests still failing due to non-existent |
…ake valid); update tests accordingly
…-compatibility and streaming use-cases
amilcarlucas
force-pushed
the
pr-qol-fixes
branch
from
e77da6d to
d6c408f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors mavlogdump.py to improve usability and consistency by restructuring the code into a more maintainable function-based architecture, consolidating output format handling, and enhancing test coverage.
Key changes:
- Refactored monolithic script into modular functions with a main
dump_log()function that can be called programmatically - Unified output path argument (
--output_pathreplacing--outputand--mat_file) and improved format options (addedndjson,pretty,types-only) - Added comprehensive test suite with ~30 test cases covering all output formats and filtering options
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| tools/mavlogdump.py | Complete refactoring into modular functions with consolidated output handling and improved argument naming |
| tests/test_mavlogdump.py | Expanded test suite from single test to comprehensive coverage of all formats and features |
Comments suppressed due to low confidence (1)
tools/mavlogdump.py:1
- The test references
mavlogdump.to_string()but this function is now imported asto_stringfrompymavlink.DFReader(line 19) and not defined in mavlogdump itself. The test will fail becausemavlogdump.to_stringdoesn't exist.
#!/usr/bin/env python3
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| with open(output_path, mode='wb' if format =='standard' else 'w') if output_path else sys.stdout as output_fh: | ||
|
|
There was a problem hiding this comment.
Complex conditional expression combining file opening and stdout handling is difficult to read and maintain. Consider splitting this into separate conditional blocks for clarity: first check if output_path exists, then determine the mode based on format.
| with open(output_path, mode='wb' if format =='standard' else 'w') if output_path else sys.stdout as output_fh: | |
| if output_path: | |
| mode = 'wb' if format == 'standard' else 'w' | |
| output_fh = open(output_path, mode) | |
| else: | |
| output_fh = sys.stdout | |
| try: |
Copilot uses AI. Check for mistakes.
| ): | ||
|
|
||
| # set up output file handler based on format and output_path | ||
| with open(output_path, mode='wb' if format =='standard' else 'w') if output_path else sys.stdout as output_fh: |
There was a problem hiding this comment.
When output_path is None and sys.stdout is used, the context manager will attempt to call __exit__ on sys.stdout, which may not behave as expected since stdout is not normally used as a context manager. This could cause issues when the with block exits. Consider using contextlib.nullcontext or separate handling for stdout.
Copilot uses AI. Check for mistakes.
|
|
||
| filename = log | ||
| mlog = mavutil.mavlink_connection(filename, planner_format=planner, | ||
| no_timestamps=no_timestamps, |
There was a problem hiding this comment.
The parameter name 'no_timestamps' in mavutil.mavlink_connection has been changed from the original 'notimestamps'. This may be a breaking change if the mavutil API hasn't been updated accordingly. Verify that mavutil.mavlink_connection accepts 'no_timestamps' as a parameter.
| no_timestamps=no_timestamps, | |
| notimestamps=no_timestamps, |
Copilot uses AI. Check for mistakes.
| def test_dump_csv_format(self): | ||
| """Test CSV format output""" | ||
| output_file = os.path.join(self.test_dir, "csv_output.csv") | ||
| output_file='testing.csv' |
There was a problem hiding this comment.
This line overwrites the temporary output_file path with a hardcoded filename 'testing.csv' in the current directory, which will leave test artifacts and potentially cause test failures in CI environments. Remove this line to use the properly scoped temporary file path.
| output_file='testing.csv' |
Copilot uses AI. Check for mistakes.
| output_fh.write('') | ||
| elif hasattr(m,"dump_verbose"): | ||
| m.dump_verbose(output_fh) | ||
| output_fh.write('') |
There was a problem hiding this comment.
Writing empty strings has no effect. If the intention is to write newlines for formatting, use '\n' instead. Otherwise, these lines should be removed.
| output_fh.write('') | |
| elif hasattr(m,"dump_verbose"): | |
| m.dump_verbose(output_fh) | |
| output_fh.write('') | |
| output_fh.write('\n') | |
| elif hasattr(m,"dump_verbose"): | |
| m.dump_verbose(output_fh) | |
| output_fh.write('\n') |
Copilot uses AI. Check for mistakes.
| output_fh.write('\n]\n') | ||
| elif format == 'types-only': | ||
| for msgType in available_types: | ||
| output_fh.write(msgType) |
There was a problem hiding this comment.
Missing newline separator between message types. Each message type should be written on a separate line for readability. Change to output_fh.write(msgType + '\\n') or similar.
| output_fh.write(msgType) | |
| output_fh.write(msgType + '\n') |
Copilot uses AI. Check for mistakes.
| if args.profile: | ||
| yappi.get_func_stats().print_all() | ||
| yappi.get_thread_stats().print_all() | ||
| output_fh.write(s) |
There was a problem hiding this comment.
Missing newline after writing the standard output message. This will cause all messages to run together on a single line when written to a file. Add '\n' to the output: output_fh.write(s + '\\n').
| output_fh.write(s) | |
| output_fh.write(s + '\n') |
Copilot uses AI. Check for mistakes.
1 participant
Various QoL fixes to make UX more consistent and produce behavior a new user might expect.