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

Parse markdown descriptions #1106

Merged
merged 26 commits into from Aug 5, 2023
Merged

Parse markdown descriptions #1106

merged 26 commits into from Aug 5, 2023

Conversation

xsebek
Copy link
Member

@xsebek xsebek commented Feb 13, 2023

  • use CommonMark to parse simple markdown AST parametrised on inline/block code
  • validate swarm code (Document Text -> Document Syntax)
  • update descriptions to use markdown with following conventions:
    • move - valid swarm code (the easy to write default)
    • wedge{=entity} - for swarm entities
    • unit{=type} - for swarm types
    • require <a> <b>{=snippet} - raw snippets for invalid code
    • Alt-G - bold for keyboard shortcuts

Copy link
Member Author

@xsebek xsebek left a comment

Choose a reason for hiding this comment

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

@byorgey its only in a MVP stage right now, but it already caught this inaccuracy:

    parse scenario "data/scenarios/Tutorials/requireinv.yaml":  FAIL
      test/integration/Main.hs:115:
      Aeson exception:
      Error in $.objectives[0].goal[0]: 1:8:
        |
      1 | require
        |        ^
      reserved word 'require' cannot be used as variable name

Because require is not a command we can not use it on its own. 😄


Thinking about this a bit, maybe I could handle reserved words. It would require (pun intended) a bit more work to highlight them properly later though. 🤔

I think we should not use them for entities though and only use code for valid code snippets.

readTerm txt & \case
Left e -> Left (T.unpack e)
Right Nothing -> Left "empty code"
Right (Just s) -> case processParsedTerm s of
Copy link
Member Author

Choose a reason for hiding this comment

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

We currently use inline code for types like dir -> cmd unit.

I think I can solve the type signatures as an alternative parse.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can make a parallel to readTerm for types, like readType = runParser (fully parseType). If readTerm fails you can just try running readType too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I turned types to "raw inlines" for now:

`cmd a`{=type}

src/Data/Text/Markdown.hs Outdated Show resolved Hide resolved
src/Data/Text/Markdown.hs Outdated Show resolved Hide resolved
src/Swarm/Game/Scenario/Objective.hs Show resolved Hide resolved
titleWidget = maybe (txt "?") withEllipsis textSource

singleGoalDetails :: GoalEntry -> Widget Name
singleGoalDetails = \case
Goal _gs obj -> displayParagraphs $ obj ^. objectiveGoal
Goal _gs obj -> displayParagraphs . map Markdown.toText $ obj ^. objectiveGoal
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to change to properly support the whole markdown text (with code) instead of a list of paragraphs.

I suppose it might make sense to use [[StreamNode]] as paragraphs so that we can add padding.

@xsebek xsebek changed the title Parse markdown goal description Parse markdown descriptions Feb 13, 2023
src/Data/Text/Markdown.hs Outdated Show resolved Hide resolved
src/Data/Text/Markdown.hs Outdated Show resolved Hide resolved
fromGeneralNode (CMark.Node p t cs) = case t of
CMark.DOCUMENT -> Node Document <$> mapM fromGeneralNode cs
CMark.PARAGRAPH -> Node Paragraph <$> mapM fromGeneralNode cs
CMark.SOFTBREAK -> leaf $ LeafText "\n" -- ?
Copy link
Member

Choose a reason for hiding this comment

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

According to https://github.github.com/gfm/#soft-line-breaks it seems like a SOFTBREAK is what we get at the end of a physical line of parsed text which does not end with two or more spaces. Typically we ignore such breaks and just reflow the text, so I would think we should not generate a LeafText "\n" here, unless I am misunderstanding something.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we might need to use a space instead of newline here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: check that spacing is preserved.

readTerm txt & \case
Left e -> Left (T.unpack e)
Right Nothing -> Left "empty code"
Right (Just s) -> case processParsedTerm s of
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can make a parallel to readTerm for types, like readType = runParser (fully parseType). If readTerm fails you can just try running readType too.

src/Swarm/Game/Scenario/Objective.hs Show resolved Hide resolved
mergify bot pushed a commit that referenced this pull request Apr 3, 2023
May be able to reduce likelihood of #1086 by having a autogenerated table of the commands and the tutorials they are introduced in.

This work is related to #1106 in that robot commands should be given distinct markup (backticks).

## Demo

    scripts/play.sh generate pedagogy

**Updated:** See [rendered output](https://gist.github.com/kostmo/d4af3a6814e65fc6d455fd34b41552d3).

## New tests

    scripts/run-tests.sh --test-arguments '--pattern "Pedagogical soundness"'

Note that the test for `crash.yaml` is currently failing because the `Give` command (and some others) are utilized in this tutorial without having been mentioned in the description or in earlier tutorials.

Additionally, the "bug" described in [this comment](#1089 (comment)) **should** be caught by these tests (i.e. fail the tests) since the embedded `solution` code in the YAML file utilizes the `give` command.  That is, if it currently weren't for [this issue](https://github.com/swarm-game/swarm/pull/1186/files#r1152789153).
xsebek and others added 5 commits August 2, 2023 12:00
xsebek and others added 3 commits August 3, 2023 03:05
@xsebek
Copy link
Member Author

xsebek commented Aug 3, 2023

Huzzah! 🥳

Screenshot 2023-08-03 at 3 04 15

Turns out CommonMark has a neat extension that allows me to annotate inline code as entities/types/invalid code snippets.
Took me a while to rewrite, but at least I don't have to study a C library to figure out how it works. 😅

@kostmo If it is OK, I would switch from CMark-GFM to CommonMark. AFAIK it was only used in one place and the change was simple one-liner.

src/Swarm/Doc/Pedagogy.hs Outdated Show resolved Hide resolved
@xsebek xsebek marked this pull request as ready for review August 3, 2023 11:18
@xsebek xsebek requested review from kostmo and byorgey August 3, 2023 11:18
@xsebek
Copy link
Member Author

xsebek commented Aug 3, 2023

@byorgey @kostmo I rewrote the implementation from CMark-GFM to CommonMark and while it is a bit of a DIY project, I hope it will make reading descriptions a little easier. 🙂

For reference, the markdown highlighting conventions I went with are:

  • move - valid swarm code (the easy to write default)
  • wedge{=entity} - for swarm entities (I generated a regex to catch all standard ones, but they are not validated)
  • unit{=type} - for swarm types (there are not so many and I do not validate them yet)
    • EDIT: this would make it easier to validate types as parsing alternatives would only report the last failure
  • require <a> <b>{=snippet} - raw snippets for invalid code
  • Alt-G - bold for keyboard shortcuts

If we want to do something else with any of those cases, they should be easy to grep and replace.

data/entities.yaml Outdated Show resolved Hide resolved
src/Data/Text/Markdown.hs Outdated Show resolved Hide resolved
txtLookups = M.fromList $ map (syntax . constInfo &&& id) commandConsts
allCode = concatMap findCode goalTextParagraphs
filterConst :: Syntax -> [Const]
filterConst sx = mapMaybe toConst $ universe (sx ^. sTerm)
Copy link
Member Author

Choose a reason for hiding this comment

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

@kostmo this is one of the little nice things a parsed code gives us. 🤩

Hopefully universe works just as well as your original implementation, but please check that I did not break it. 😅

src/Swarm/Game/Failure.hs Show resolved Hide resolved
src/Swarm/TUI/View/Objective.hs Outdated Show resolved Hide resolved
swarm.cabal Outdated Show resolved Hide resolved
@byorgey
Copy link
Member

byorgey commented Aug 4, 2023

Nice! I'm working on a detailed review, hopefully finished later today.

data/entities.yaml Outdated Show resolved Hide resolved
data/entities.yaml Show resolved Hide resolved
data/entities.yaml Outdated Show resolved Hide resolved
data/entities.yaml Outdated Show resolved Hide resolved
data/scenarios/Challenges/Sliding Puzzles/3x3.yaml Outdated Show resolved Hide resolved
data/scenarios/Tutorials/requireinv.yaml Outdated Show resolved Hide resolved
src/Swarm/Game/Failure.hs Show resolved Hide resolved
src/Data/Text/Markdown.hs Outdated Show resolved Hide resolved
data/scenarios/Tutorials/craft.yaml Show resolved Hide resolved
-- | Get chunks of nodes not exceeding length and broken at word boundary.
--
-- The split will end when no more nodes (then words) can fit or on 'ParagraphBreak'.
chunksOf :: Int -> [StreamNode] -> [[StreamNode]]
Copy link
Member

Choose a reason for hiding this comment

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

It is a shame we seem to be reimplementing some of the functionality of https://hackage.haskell.org/package/word%2Dwrap ...

Copy link
Member Author

Choose a reason for hiding this comment

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

We are doing slightly more - we have annotated chunks of text, which we break while keeping the annotation.

Maybe this could be generalised as some kind of traversal?

xsebek and others added 7 commits August 5, 2023 16:07
Co-Authored-By: Brent Yorgey <byorgey@gmail.com>

Co-authored-by: Brent Yorgey <byorgey@gmail.com>
Co-Authored-By: Brent Yorgey <byorgey@gmail.com>

Co-authored-by: Brent Yorgey <byorgey@gmail.com>
Co-authored-by: Restyled.io <commits@restyled.io>
@xsebek xsebek added the merge me Trigger the merge process of the Pull request. label Aug 5, 2023
@mergify mergify bot merged commit 1eb2f9c into main Aug 5, 2023
10 checks passed
@mergify mergify bot deleted the markdown-code branch August 5, 2023 15:39
@byorgey
Copy link
Member

byorgey commented Aug 5, 2023

🚀

@byorgey
Copy link
Member

byorgey commented Aug 5, 2023

Hmm, it doesn't seem to be working for me. After pulling main and rebuilding, I get this:

markdown

@xsebek
Copy link
Member Author

xsebek commented Aug 5, 2023

@byorgey odd, it looks like if you used old exe with new data. 😕 It worked fir me on the move tutorial, but I did not check alk of them.

I will test it with autoplay and add a script to go through all of them.

@byorgey
Copy link
Member

byorgey commented Aug 5, 2023

Oh, I see, the goal dialog is formatted properly, but the formatting doesn't show up when you look at the description of an entity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highlight commands in descriptions Check code examples in entity descriptions as part of CI
3 participants