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.

Sign up for GitHub

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

Jump to bottom

lcm-logger: Fix -l option #465

Merged
merged 3 commits into from
Aug 6, 2023
Merged

lcm-logger: Fix -l option #465

merged 3 commits into from
Aug 6, 2023

Conversation

judfs
Copy link
Contributor

@judfs judfs commented Jul 17, 2023

fix #463

For good measure, I sorted the short optstring and option array as the values need to be clearly unique.

[getopt is a funky api]

@judfs
lcm-logger: Fix -l option
fca4e3e
@judfs
Ran formating
5338826 
	modified:   lcm-logger/glib_util.c
	modified:   lcm-logger/lcm_logger.c
	modified:   lcm/eventlog.c
	modified:   lcm/ioutils.h
	modified:   liblcm-test/lcm-logfilter.c
	modified:   test/c/common.c
	modified:   test/cpp/common.cpp
@judfs
Copy link
Contributor Author

judfs commented Jul 17, 2023 •
edited
Loading

hmm...

./format_code.sh --check
FORMATTING OK!

clang-format --version
clang-format version 10.0.0-4ubuntu1

(ubuntu 20.04)

CI seems to disagree

@judfs
Copy link
Contributor Author

judfs commented Jul 18, 2023

Also: --flush-interval=MS provides the short option -u without calling it out in the help. Not a very descriptive letter. Might be better to remove than to document.

@nosracd nosracd mentioned this pull request Aug 5, 2023
Merged
@nosracd
Copy link
Contributor

nosracd commented Aug 5, 2023

hmm...

./format_code.sh --check
FORMATTING OK!

clang-format --version
clang-format version 10.0.0-4ubuntu1

(ubuntu 20.04)

CI seems to disagree

Sorry about that. I've attempted to improve the situation in #466. In the meantime, could you try rerunning with clang-format-12 installed as clang-format? Or, feel free to cherry-pick 450a380 if that's easiest.

nosracd
nosracd approved these changes Aug 5, 2023
View reviewed changes
Copy link
Contributor

@nosracd nosracd left a comment

Choose a reason for hiding this comment

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

The non-format-related changes look good to me.

@nosracd nosracd merged commit c409e91 into lcm-proj:master Aug 6, 2023
11 checks passed
@judfs judfs deleted the fix-463 branch August 7, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nosracd nosracd nosracd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lcm-logger: -l flag doesn't work
2 participants
@judfs @nosracd