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 optional serde support #63

Closed
wants to merge 2 commits into from

Conversation

not-my-profile
Copy link
Contributor

@not-my-profile not-my-profile commented Oct 19, 2024

Resolves #61.

Copy link
Owner

@hellux hellux left a comment

Choose a reason for hiding this comment

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

This looks good, the JSON output in the web demo came in handy already.

There is some awkward resulting JSON since the types
were not really designed with this direct serialization in mind. I am
not sure how much we want to massage the output as not to stray away too
far from the actual types.

  • The Attributes object is quite representative of the original source,
    e.g

    {a=b .b .b}
    

    yields

    [
      [
        {
          "Pair": {
            "key": "a"
          }
        },
        "b"
      ],
      [
        "Class",
        "b"
      ],
      [
        "Class",
        "c"
      ]
    ]

    It would be possible to instead use the output from
    Attributes::unique_pairs which should instead yield:

    {
        "a": "b",
        "class": "b c"
    }
    

    I'm guessing that would be more convenient to work with. You would
    lose the ability to use comments, though.

    The deserialization would also need some manual implementation. I
    guess e.g. {"class": "a b c"} could be turned into {.a .b .c}
    and {"id": "id"} into {.id} while all remaining keys would just
    become Pair variants.

  • Tuples are kind of awkward in JSON:

    • Start:

      "Start": [
        "Paragraph",
        []
      ]

      would probably be better represented with

      "Start": {
          "container": "Paragraph",
          "attributes": []
      }
    • Link/Image:

      "Link": [
        "https://example.com",
        {
          "Span": "Inline"
        }
      ]

      could be

      "Link": {
        "url": "https://example.com",
        "type": {
          "Span": "Inline"
        }
      }
    • ThematicBreak:

      [
        {
          "ThematicBreak": []
        }
      ]

      It is not obvious that the empty list is attributes, so it
      could be

      [
        {
          "ThematicBreak": {
              "attributes": []
          }
        }
      ]
  • There is a lot of nesting in general, it might be possible to do some
    flattening.

    • Some nested enums could maybe be flattened, e.g.

      "Link": [
        "url",
        {
          "Span": "Inline"
        }
      ]

      could be just

      "Link": [
        "url",
        "Inline"
      ]

      Does not work for ListKind though, as two of the variants use the
      ListBulletType enum.

    • Have you considered using internally tagged enums?
      Although, not sure if it is possible to specify the names
      for tuple variant elements? Some could potentially benefit from
      being converted to structs, though.

      E.g. lists could benefit from internal tagging to reduce the nesting:

      "List": {
        "kind": {
          "Ordered": {
            "numbering": "Decimal",
            "style": "Period",
            "start": 1
          }
        },
        "tight": true
      }

      could instead be

      "List": {
        "kind": {
          "type": "Ordered"
          "numbering": "Decimal",
          "style": "Period",
          "start": 1
        },
        "tight": true
      }

      and with flattening of kind:

      "List": {
        "type": "Ordered"
        "numbering": "Decimal",
        "style": "Period",
        "start": 1
        "tight": true
      }
  • Do we want to use lowercase names for all types? The PascalCase looks
    a little bit odd in JSON. On the other hand, maybe lowercase looks
    weird in other serialized formats?

Again, not sure how much pre/post-processing we want to perform, just
pointing out some things that seem potentially awkward. You may
have a better idea what would actually improve the experience for your
use case at least.

Also, if something takes too much effort to implement, as-is still
better than nothing. I might also be to help out if we both agree that
something would be an improvement.

@hellux
Copy link
Owner

hellux commented Oct 19, 2024

Also, could spans be useful to have in the JSON output? I guess it would
require implementing Serialize/Deserialize for (Event, Range<usize>). If we were
to use internal tagging, I guess we could just add start and end fields to
every Event object.

Currently simply a becomes

[
  {
    "Start": [
      "Paragraph",
      []
    ]
  },
  {
    "Str": "a"
  },
  {
    "End": "Paragraph"
  }
]

With internal tagging it could be:

[
  {
    "type": "Start":
    "container":  {
      "type": "Paragraph"
    },
    "attributes": []
  },
  {
    "type": "Str",
    "content": "a"
  },
  {
    "type": "End",
    "container": {
      "type": "Paragraph"
    }
  }
]

For (Event, Range<usize>) it would then be:

[
  {
    "type": "Start":
    "container": {
      "type": "Paragraph"
    },
    "attributes": []
    "src_start": 0,
    "src_end": 0,
  },
  {
    "type": "Str",
    "content": "a"
    "src_start": 0,
    "src_end": 1,
  },
  {
    "type": "End",
    "container": {
      "type": "Paragraph"
    }
    "src_start": 1,
    "src_end": 1,
  }
]

@not-my-profile
Copy link
Contributor Author

Thanks for your detailed feedback. I'm sorry that I didn't respond. For the project that I was needing the events for I have realized that I need more fine-grained events anyway (#78). djot.js currently works fine for me there but I now have again another project where I'm again interested in using jotdown (this time I'm interested in an AST though).

Regarding:

I guess it would require implementing Serialize/Deserialize for (Event, Range)

I just wanted to note that that's impossible in Rust since you cannot implement a foreign trait for a foreign type and tuples are always foreign.

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.

Serde support for Event
2 participants