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

Display value for all childless nodes during info #1687

Merged
merged 4 commits into from
Jan 15, 2024

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Nov 15, 2023

For childless (leaf) nodes, show the str value (when show_values is True). This allows objects like Time to display their value.

For example, on main when running asdftool info some_roman_file.asdf the output will contain:

  │ │ ├─start_time (Time)
  │ │ ├─mid_time (Time)
  │ │ ├─end_time (Time)

With this PR the same file contains the output:

  │ │ ├─start_time (Time): 2021-01-01T00:00:00.000
  │ │ ├─mid_time (Time): 2021-01-01T00:01:16.020
  │ │ ├─end_time (Time): 2021-01-01T00:02:32.040

This PR also deprecates the now unused asdf.util.is_primitive.

Fixes #1686

@@ -633,6 +633,9 @@ def __init__(self):
def __asdf_traverse__(self):
return {"the_meaning": self.the_meaning, "clown": self.clown, "recursive": self.recursive}

def __str__(self):
return "rec ref"
Copy link
Contributor Author

@braingram braingram Nov 15, 2023

Choose a reason for hiding this comment

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

This was added because with this PR (and without this change) the assert in test_recursive_info_object_support:

assert "recursive reference" in captured.out

would fail. This was due to the output line being truncated due to this test object (RecursiveObjectWithInfoSupport) not having a __str__ and defaulting to the verbose <asdf._tests.test_info.RecursiveObjectWithInfoSupport object at 0x11cba1120>. This pushes the output line beyond the max_cols limit and cuts off the recursive reference string (that is appended to the end of the long line). This test failure could have also been fixed by increasing max_cols.

asdf/_display.py Outdated
@@ -236,7 +235,7 @@ def _render_node(self, info, active_depths, is_tail):
)

if info.info is not None:
line = line + format_faint(format_italic(" # " + info.info))
line = line + format_faint(format_italic(" # " + info.info.strip()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is to deal with info that contains newlines like the exposure in RAD. See #1686 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also replace any embedded newlines with spaces? In case for some reason the string value looks like this:

Hi

Mom!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tldr; yaml is messy, I think the schemas would be better off not using literal | blocks and the other multiline options seems to strip embedded newlines

I think this is already being done (in some places). For example:

  │ │ ├─obs_id (str): 0000101001001001001011010001 # Programmatic observation identifier. The format is 'PPPPPCCAAASSSOOOVVVggsaaeeee' where 'PPPPP' is the Program, 'CC' is the execution plan, 'AAA' is the pass, 'SSS' is the segment, 'OOO' is the Observation, 'VVV' is the Visit, 'gg' is the visit file group, 's' is the visit file sequence, 'aa' is the visit file activity, and 'eeee' is the exposure ID. The observation ID is the complete concatenation of visit_id + visit_file_statement (visit_file_group + visit_file_sequence + visit_file_activity) + exposure.

Is pulling in the info from this multi-line title here: https://github.com/spacetelescope/rad/blob/main/src/rad/resources/schemas/observation-1.0.0.yaml#L10-L16

I think the issue is the difference between:

this: description
        with 2 lines

and

this: |
    description
    with 2 lines

The first is read by pyyaml as a single string with no newlines:

{'this': 'description with 2 lines'}

whereas the second preserves the embedded and trailing newlines

{'this': 'description\nwith 2 lines\n'}

I think that the use of these literal blocks (with the |) might be considered bugs in the schema (these could be 'folded', see below, or multiline formatted like the first example).

To further complicate things the yaml "folded" style retains only the trailing newline:

this: >
    description
    with 2 lines
{'this': 'description with 2 lines\n'}

Although this can be controlled using a "Block Chomping Indicator"

this: >-
    description
    with 2 lines
{'this': 'description with 2 lines'}

Copy link
Contributor

Choose a reason for hiding this comment

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

How bout we display something like:

description... (value continues onto additional line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in favor of that change or something similar to truncate long info strings. I just checked and the "info" portion of the line is included in the "max_cols" check which truncates long lines.

I dropped the strip change from this PR to keep it focused on fixing the Time issue and opened a separate issue to track the line formatting: #1712

Copy link
Contributor

@eslavich eslavich left a comment

Choose a reason for hiding this comment

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

LGTM

@braingram braingram merged commit d9d9974 into asdf-format:main Jan 15, 2024
48 checks passed
@braingram braingram deleted the time_info branch January 15, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

asdftool repr for Time objects
2 participants