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

Add support for literat Cryptol with RST #1443

Merged
merged 3 commits into from
Sep 28, 2022
Merged

Add support for literat Cryptol with RST #1443

merged 3 commits into from
Sep 28, 2022

Conversation

yav
Copy link
Member

@yav yav commented Sep 27, 2022

This implements supports for literate Cryptol with RST, see #1441

{-# LANGUAGE OverloadedStrings, Safe, PatternGuards #-}
{-# LANGUAGE OverloadedStrings, PatternGuards #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the code in this module no longer Safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is still Safe. I think we've been using Safe incorrectly, probably due to a historical misunderstanding on my part about what the pragma means, so I thought I might as well start fixing things slowly. In particular, GHC can already infer which modules are safe and which are not, without the pragma. Instead, the pragma explicitly asks GHC to check if a module is safe, and complain if it is not. So I don't think we need to annotate every single module with Safe, but rather we should add it to modules where we want this checked. We currently don't really have a use case for this feature, so I am not sure which modules should have the annotation...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. I don't have too strong of an opinion about Safe myself (if anything, I am slightly biased against it), so I'm happy to defer to your judgment about whether they should be kept or removed.

rst :: [Text] -> [Block]
rst = comment []
where
isBeginCode l = case filter (not . Text.null) (Text.splitOn " " l) of
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should use Text.split isSpace instead, both for consistency with isEmpty/isCode and to catch other whitespace characters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, will change.

l : ls1 | isBeginCode l -> codeOptoins (l : acc) ls1
| otherwise -> comment (l : acc) ls1

codeOptoins acc ls =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
codeOptoins acc ls =
codeOptions acc ls =

comment acc ls =
case ls of
[] -> mk Comment acc
l : ls1 | isBeginCode l -> codeOptoins (l : acc) ls1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
l : ls1 | isBeginCode l -> codeOptoins (l : acc) ls1
l : ls1 | isBeginCode l -> codeOptions (l : acc) ls1

case ls of
[] -> mk Comment acc
l : ls1 | isEmpty l -> mk Comment (l : acc) ++ code [] ls1
| otherwise -> codeOptoins (l : acc) ls1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| otherwise -> codeOptoins (l : acc) ls1
| otherwise -> codeOptions (l : acc) ls1

rst = comment []
where
isBeginCode l = case filter (not . Text.null) (Text.splitOn " " l) of
["..", "code::", "cryptol"] -> True
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should recognize code-block:: and sourcecode:: in addition to code::? The first two are officially recognized directives in Sphinx.

In fact, code is technically a different thing altogether, although I don't know if strict adherence to Sphinx's rules is a goal for Cryptol.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are quite right, I was reading the docs for code-block and I guess auto-corrected it code :)

@podhrmic
Copy link

@yav I tested it and it correctly parses all rst examples I have available. Thanks for putting it together!

@yav yav merged commit 94283f2 into master Sep 28, 2022
@RyanGlScott RyanGlScott deleted the T1441 branch March 22, 2024 14:49
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.

3 participants