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

Add display text if there is no format string #135

Merged
merged 2 commits into from
Aug 23, 2023

Conversation

SMorettini
Copy link
Contributor

@SMorettini SMorettini commented Aug 3, 2023

Originating Project/Creator
Affected Component data_types
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

This merge request add a display_text for events having arguments but not a format_string.

Rationale

Before the arguments of an event without a format_string were not logged or printed everywhere. This pull request will enable the visualization of the argument also if the format_string is not set.

An event with no format_string but three arguments(an integer, an enum and a float) will contain in the column Event Description of gds: [{'My_integer_arg': 123}, {'My_enum_arg': 'VALUE_OF_ENUM'}, {'My_float_arg': 12.34}].

@bocchino
Copy link
Collaborator

bocchino commented Aug 3, 2023

Can you explain the motivation for this PR a bit more?

  1. Event specifiers in FPP are required to have format strings. What is the scenario where an event specifier without a format string is presented to the GDS?
  2. What is the reason for handling just this case of an ill-specified event (n > 0 args and no format specifier)? What about an event that has n args and m replacement fields in its format specifier, for n =/= m? This is also disallowed by FPP.

@SMorettini
Copy link
Contributor Author

About point 1: I thought about this change because I was using gds to communicate with a subsystem of my project that doesn't use Fprime. So I created the xml dictionary manually and I didn't put the format string.
About point 2: I didn't consider that case.

If the idea is to support only the dictionary produced by fpp, then we can close this pull request directly.

@LeStarch
Copy link
Collaborator

I see this PR as patching an edge case. What to do when format-string == ""? In FPP the user would have to explicitly place an empty string which likey means that was intended...but that is not useful.

@LeStarch
Copy link
Collaborator

Outside of FPP (dictionary) it means the text was hand-edited and the format string was forgotten.

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Rationale: if a user explicitly wants an empty format string then " " may be used. This fixes the stated issue and prints something if the format string was forgotten.

@LeStarch LeStarch merged commit f226db2 into nasa:devel Aug 23, 2023
12 checks passed
@SMorettini SMorettini deleted the Add-default-display-text-for-events branch August 24, 2023 07:26
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.

None yet

4 participants