search

gmarpons / asciidoc-hs

AsciiDoc parser that can be used as a Pandoc front-end, written in Haskell
BSD 3-Clause "New" or "Revised" License
47 stars 4 forks source link

Some additions test #1

Closed guaraqe closed 3 years ago

guaraqe commented 3 years ago

I tried to add a block (page breaks) and two inlines (subscripts and superscripts). On the good side, I think I did something that is minimally functional without knowing the code base, that's good!

However, it was not always evident to know where to go in the file to focus on the functionality that matters. In these cases, I like to split the file into sections, using a line with 80 - and a section title below. This is not perfect, but forces some kind of explicit organization in the code.

Some more specific difficulties I had:

On a side note, I lowered some of the Cabal bounds, to make it possible to build on my system, that is a little outdated. This builds correctly, so that's positive - that makes it easier for contributors. I would recommend to add a stack.yaml to the repository, many people use Stack, and Stackage is a good reference on what's the currently acceptable bleeding edge.

gmarpons commented 3 years ago

Hi @guaraqe, many thanks for your comments and contributions!

I'll try to answer you point by point in the next hours.

gmarpons commented 3 years ago

I did no changes on how Pandoc interprets the new things I added. Maybe there is a catch-all pattern somewhere? Giving pattern-matching errors when adding new constructors is good.

I do agree, but I get the following incomplete pattern warning when cabal build, which is expected:

src/Text/AsciiDoc/Pandoc.hs:33:17: warning: [-Wincomplete-patterns]
    Pattern match(es) are non-exhaustive
    In a case alternative:
        Patterns not matched:
            StyledText Subscript
            (ParameterList (Data.Text.Internal.Text _ _ _))
            (Data.Text.Internal.Text _ _ _) (_ GHC.Base.:| _)
            (Data.Text.Internal.Text _ _ _)
            StyledText Superscript
            (ParameterList (Data.Text.Internal.Text _ _ _))
            (Data.Text.Internal.Text _ _ _) (_ GHC.Base.:| _)
            (Data.Text.Internal.Text _ _ _)
   |
33 | convertInline = \case
   |                 ^^^^^
...
gmarpons commented 3 years ago

I tried to add a block (page breaks) and two inlines (subscripts and superscripts). On the good side, I think I did something that is minimally functional without knowing the code base, that's good!

Yes, additions were correct.

Unfortunately, I discovered that my implementation of block parsing was not going to work in the presence of includes.

I've re-implemented block parsing in a new branch. I explain the motivation for the new implementation in the wiki.

However, it was not always evident to know where to go in the file to focus on the functionality that matters. In these cases, I like to split the file into sections, using a line with 80 - and a section title below. This is not perfect, but forces some kind of explicit organization in the code.

I hope the new implementation is more clear and the code better organized. I've divided the module declaration into haddock-aware sections.

gmarpons commented 3 years ago

I didn't know how to manage BlockPrefixItem. Could this be standardized in some way? I just added an empty list, but I am certainly ignoring information.

The new implementation for blocks has renamed this data type to MetadataItem, and I think that its purpose is more clear.

I used Scope, but by supposing what it means. Some documentation on these fundamental types, that will certainly be manipulated by contributors, would be very useful.

Yes, documentation is lacking in this regard, I'll try to improve that. I'm still looking for a better name than Scope.