Skip to content

Conversation

medley56
Copy link
Member

  • Avoid passing namespace dicts down through all from_xml methods in favor of predefining a custom Element class that contains the namespace
  • Convert internal parameter types, parameters, and containers to set objects with read only dict accessor properties.
  • Increase performance by minimizing broad XML find and findall calls.

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 February 15, 2025 18:35
@medley56 medley56 force-pushed the handle-xml-namespaces-correctly branch 4 times, most recently from 54e5a15 to e7f220d Compare February 15, 2025 20:08
Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

I don't know enough about namespaces or XML, so most of my comments are just my ignorance of it all. Feel free to dismiss or ignore. Overall, I really like that this gets rid of the ns showing up everywhere and think it will simplify things moving forward!

@medley56 medley56 force-pushed the handle-xml-namespaces-correctly branch from 79aef35 to 87288dd Compare February 16, 2025 14:54
Copy link

codecov bot commented Feb 16, 2025

Codecov Report

Attention: Patch coverage is 93.52751% with 20 lines in your changes missing coverage. Please review.

Project coverage is 91.82%. Comparing base (5c40558) to head (ecf6f08).

Files with missing lines Patch % Lines
tests/unit/test_xtce/test_parameter_types.py 76.66% 7 Missing ⚠️
space_packet_parser/common.py 95.65% 2 Missing ⚠️
space_packet_parser/xtce/containers.py 89.47% 2 Missing ⚠️
space_packet_parser/xtce/definitions.py 94.87% 2 Missing ⚠️
tests/unit/test_xtce/test_calibrators.py 84.61% 2 Missing ⚠️
tests/unit/test_xtce/test_encodings.py 90.47% 2 Missing ⚠️
space_packet_parser/xtce/calibrators.py 92.30% 1 Missing ⚠️
space_packet_parser/xtce/comparisons.py 94.44% 1 Missing ⚠️
tests/unit/test_xtce/test_comparisons.py 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           main     #143       +/-   ##
=========================================
+ Coverage      0   91.82%   +91.82%     
=========================================
  Files         0       35       +35     
  Lines         0     2543     +2543     
=========================================
+ Hits          0     2335     +2335     
- Misses        0      208      +208     

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

Copy link

codecov bot commented Feb 17, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

To me, this looks super helpful and is much appreciated to remove all the required "ns" passing. Thank you for looking into this. All of my minor code-formatting comments have been addressed. I think the logic seems sound, but I don't have enough experience to fully verify that is the best approach. I think we should get this in sooner rather than later due to the amount of code it touches by just removing "ns=" in so many places. We can iterate on the little things later because it is mostly non-user facing.

@medley56 medley56 merged commit fd157b8 into main Feb 17, 2025
19 checks passed
@medley56 medley56 deleted the handle-xml-namespaces-correctly branch February 17, 2025 19:46
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.

3 participants