-
Notifications
You must be signed in to change notification settings - Fork 4
.janno column sorting with special treatment of _Note columns
#358
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
Conversation
…lumns when writing .janno files
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## poseidon300cols #358 +/- ##
===================================================
+ Coverage 55.82% 55.90% +0.07%
===================================================
Files 33 33
Lines 5123 5109 -14
Branches 571 568 -3
===================================================
- Hits 2860 2856 -4
+ Misses 1692 1685 -7
+ Partials 571 568 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
stschiff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, looks all good. I've made some cosmetic suggestions which you can feel free to ignore. I don't quite get why findspot with its recursion doesn't loop endlessly, given that there is no termination criterion... but I must be overlooking something, as I've tentatively added a test with a fake note ("Blabla_Note") and it does finish. Maybe you can explain. Otherwise looks good!
| findSpot i x | ||
| | removeSuffix i == x = LT | ||
| | removeSuffix x == x = GT | ||
| | otherwise = findSpot i (removeSuffix x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why does this function not run endlessly in case there is no match? You did not put in a stopping criterion in case there is no match, did you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

This would enable the automatic sorting of
_Notecolumns in .janno file writing as raised in poseidon-framework/poseidon-schema#110. With this change_Notecolumns don't have to be definied explicitly any more in the Poseidon schema, but can just be added wherever the user needs them. If this gets merged we can adjust the schema draft for v3.0.0 accordingly, and also update the .janno and trident documentation on the website eventually.I think the algorithm works excatly as expected. My test to prove it is not particularly clever, though.
Note that this PR is meant to be merged into #357, which is also still under review. That's how confident I am 😉