Skip to content

Add Manual Storable Instances for DPIXid and VersionInfo (#26) #42

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

Merged

Conversation

tusharad
Copy link
Contributor

@tusharad tusharad commented Nov 21, 2024

This PR addresses issue #26 by implementing manual Storable instances for DPIXid and VersionInfo, instead of relying on GStorable.
I kindly request a review of Implementation (whether it aligned with best practices).

If the approach is deemed correct, I intend to implement manual Storable instances for other types as part of this refactoring effort. Feel free to suggest any changes, optimizations, or best practices that could enhance the implementation. Thank you for your time and guidance!

@apkhandkar @dmjio

@dmjio
Copy link
Collaborator

dmjio commented Nov 27, 2024

@tusharad adding a test would be helpful

@tusharad
Copy link
Contributor Author

@dmjio

Added round-trip test cases for VersionInfo and DPIXid to ensure their Storable instances are working correctly. For this, I had to expose DPIXid from the Transaction module to make it accessible for testing. Also added Eq instance for DPIXid (Not sure why it was missing previously, but it is necessary for the test assertions). This commit is related to Issue #20, which discusses adding round-trip instance tests. Let me know if anything else required.

Thanks.

@dmjio
Copy link
Collaborator

dmjio commented Nov 28, 2024

LGTM 👍🏼

@dmjio dmjio merged commit 1edace0 into haskell-oracle:master Nov 28, 2024
1 check passed
@tusharad tusharad deleted the feature/optimizing_storable_instances branch November 30, 2024 10:13
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.

2 participants