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

Remove bitstring dependency #59

Merged
merged 2 commits into from
Aug 31, 2024

Conversation

greglucas
Copy link
Collaborator

@greglucas greglucas commented Aug 31, 2024

Title of PR

This removes the bitstring dependency and all bitstring inputs allowed with it. You can pass in the raw objects instead of the bitstring objects now.

This also removes the legacy_parser method which was kept around to support csv parsing. This has been removed in favor of converting the csv packet definitions into the XTCE format instead.

This sits on top of #58 so there wasn't as much thrashing going on. Only the second commit is the removal portion.

NOTE: I have removed the entire csvdef module. I am not sure if you want to keep that around or not. This is definitely the heavy-handed approach of just removing all of the legacy things and moving on in a major version bump.

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

@greglucas greglucas added the breaking This involves a breaking change label Aug 31, 2024
@medley56
Copy link
Owner

Looks like there are some issues with the tests hanging around still. Some of the integration tests for CSV parsing we will either need to remove entirely or we will need to come up with an XTCE definition for the previously used CSV files.

@greglucas
Copy link
Collaborator Author

Looks like there are some issues with the tests hanging around still. Some of the integration tests for CSV parsing we will either need to remove entirely or we will need to come up with an XTCE definition for the previously used CSV files.

For some reason, I thought the csv parsing required bitstring which is why I removed it earlier. It turns out it doesn't, so I've now reverted all of the CSV changes and made this just a removal of bitstring now. This is much cleaner and can take place in two steps with a future removal of the legacy csv parser.

This removes the bitstring dependency and all bitstring inputs
allowed with it. You can pass in the raw objects instead of the
bitstring objects now.

All internal bookkeeping is done with a PacketData class that
is just a thin wrapper around bytes data to keep track of the
amount of data read.
Copy link
Owner

@medley56 medley56 left a comment

Choose a reason for hiding this comment

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

This is awesome. Unless my recollection deceives me, the speed boost is perceptible in the integration tests.

docs/source/changelog.md Outdated Show resolved Hide resolved
@medley56 medley56 merged commit 1e8d1cf into medley56:main Aug 31, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This involves a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants