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 spp.create_packet_list function #160

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

medley56
Copy link
Member

@medley56 medley56 commented Mar 8, 2025

Checklist

  • Changes are fully implemented without dangling issues or TODO items
  • Deprecated/superseded code is removed or marked with deprecation warning
  • Current dependencies have been properly specified and old dependencies removed
  • New code/functionality has accompanying tests and any old tests have been updated to match any new assumptions
  • The changelog.md has been updated

@medley56 medley56 requested a review from greglucas March 8, 2025 00:43
@medley56 medley56 added the enhancement New feature or request label Mar 8, 2025
@medley56 medley56 force-pushed the add-list-packets-function branch from 4380fab to 85e4b51 Compare March 8, 2025 00:50
Copy link

codecov bot commented Mar 8, 2025

Codecov Report

Attention: Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.49%. Comparing base (7d9468a) to head (b0ab4b6).

Files with missing lines Patch % Lines
space_packet_parser/xarr.py 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
+ Coverage   93.43%   93.49%   +0.05%     
==========================================
  Files          37       37              
  Lines        2605     2627      +22     
==========================================
+ Hits         2434     2456      +22     
  Misses        171      171              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +37 to +38
packet_generator_kwargs : Optional[dict]
Keyword arguments passed to `XtcePacketDefinition.packet_generator()`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to add this option if we are thinking about getting rid of the generator on the definition? I'm wondering if we want to accept a generator instead here, or call this method create_ccsds_packet_list or something like that?

I'm still torn on how this should all behave and how to make it simple and general at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we accept a custom generator function then I think we want to allow the user to pass kwargs to it. We certainly need it for the ccsds_generator, right? One option could be to disallow additional kwarg passing and tell the user they have to pass the generator as a functools.partial with options already set. I'm not sure what is easier for people.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking broader about what it means to be generic to any packet type with XTCE parsing. We probably shouldn't provide a generator like I mentioned, instead the XTCE parsing would yield the packets that it produces during its parsing. This would also mean removing the ccsds_generator from the definition.packet_generator() routine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Linking to #140.
I'm still not understanding how we would decide how many bytes to read from the source before we instantiate a Packet for parsing without letting users define that logic for themselves. For CCSDS it's obvious but it's not obvious to me how we would do it without a CCSDS header to tell us how many bytes are in the packet. e.g. How many bytes should go in the binary_data attribute of a new Packet object?

@medley56 medley56 force-pushed the add-list-packets-function branch from 55d0152 to d0a1873 Compare March 8, 2025 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants