-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add support for block and scripts #75
Conversation
c07d42e
to
3f28d89
Compare
f9a5461
to
9f0a284
Compare
9f0a284
to
a9fa404
Compare
Rebased & squashed |
@harendra-kumar @adithyaov could you have a look at it? |
genBlocksModule moduleName = done <$> Fold.foldl' step initial | ||
where | ||
|
||
done (blocks, defs, ranges) = let ranges' = reverse ranges in unlines |
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.
Can probably use Streamly.Internal.Unicode.String.str
.
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.
I find reference to neither the module nor the function on Hackage. The parser is also far from optimized in many other places.
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.
Apologies for the extremely delayed review.
Overall, LGTM!
@harendra-kumar I will merge this soon if there is no further 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.
- The script file is quite big, this will also make the compiled library size big, do we want to put it into a separate package like name/name-aliases?
- To avoid making too many packages we can possibly have just two packages, we can call the lightweight package as unicode-data-core and bundle everything including unicode-data-core in the all-inclusive unicode-data package.
- What is difference in "script" and "inScript" APIs? can
inScript
be derived fromscript
? Similar question forblock
andinBlock
.
@wismill I think the review got delayed because this was a wip earlier and I missed when it became ready for review. If you are not getting attention for some PR please ping me or send an email requesting review on my personal email id harendra.kumar at gmail. |
I do not have a strong opinion on this. I like the idea of Let’s merge this PR and discuss the split in #82.
Fixed: |
0251b9b
to
303a5ce
Compare
Rebased |
@harendra-kumar I splited scripts into a new package |
Add support for block and scripts.