-
Notifications
You must be signed in to change notification settings - Fork 4
implemented new restricted ByteString for PoseidonID and GroupNames #362
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
base: Schema_300_dev
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## Schema_300_dev #362 +/- ##
==================================================
+ Coverage 56.83% 56.89% +0.05%
==================================================
Files 33 33
Lines 4993 5016 +23
Branches 546 550 +4
==================================================
+ Hits 2838 2854 +16
- Misses 1609 1612 +3
- Partials 546 550 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
OK, I've added tests. This is ready for review. |
|
Nice! The conversions between String, Bytestring and Text are sometimes a bit awkward, but I still think you made the right decisions by choosing Bytestring as the main format. The code looks fine 👍 I ran commmunity-archiveaadr-archiveminotaur-archiveHow should we approach this reality? We still have to be able to read this old data. Maybe the strict validation should only happen for Poseidon v3.0.0+ packages? |
|
I had an alternative idea: trident could turn every unexpected character to |
|
Update on this issue from today's meeting: We've become unsure on whether it is still a good idea to introduce this restriction with Poseidon 3.0. To discuss this, I have opened a PR on the schema-repo |
Previously, our type for
jPoseidonIDin the Janno Structure wasString. I have now changed this to anewtypebased on ByteString. I have also changedGroupNameto now be also based on ByteString, which makes it more compatible with sequence-formats.I've implemented a smart constructor via
Makeable, which now checks for illegal characters.I still have to implement some tests, so not ready for review just yet.