kowainik / crocodealer

🐊 Manage GitHub organization files, labels, issues
https://kowainik.github.io/projects/crocodealer
Mozilla Public License 2.0
9 stars 4 forks source link

Implement TOML parser for the config using tomland #6

Closed chshersh closed 5 years ago

chshersh commented 5 years ago

~Blocked by #3~

Nimor111 commented 5 years ago

Hey, since the issue this was blocked by is resolved, is this up for grabs?

chshersh commented 5 years ago

@Nimor111 sure, you can take this issue! All data types are implemented, so nothing blocks writing the config for them :slightly_smiling_face:

Nimor111 commented 5 years ago

Hey, I might need a little help on this one. :/ So my idea for the toml config is the following:

username = "my-user"
repository = "https://github.com/my-org/my-repo"

[[labelRule]]
  [labelRule.create]
  name = "label1"
  description = "a cool label"
  hexColor = "#000000"

[[labelRule]]
  [labelRule.override]
  old = "old label name"
  new = "new label name"

[[ignoredRepository]]
repo = "https://github.com/my-org/my-ignored-repo"

[[ignoredRepository]]
repo = "https://github.com/my-org/my-other-ignored-repo"

The username, repo and ignored repos were trivial enough to implement, but the label rule is really giving me trouble. Since it's a sum type, I figured it would need a custom BiMap as shown here. But since the Override constructor of the LabelRule has two arguments I can't really figure out how to write the BiMap for it:

-- Here it's ok since it's only one argument
_Create :: TomlBiMap LabelRule Label
_Create = Toml.prism Create matchCreate

matchCreate :: LabelRule -> Either TomlBiMapError Label
matchCreate (Create label) = Right label
matchCreate e              = Toml.wrongConstructor "Create" e

_Override :: TomlBiMap LabelRule ???
_Override = Toml.prism Override matchOverride

matchOverride :: LabelRule -> Either TomlBiMapError ???
matchOverride (Override old new) = Right ???
matchOverride e                  = Toml.wrongConstructor "Override" e

And finally, i'm not really clear on how to write a BiMap to AnyValue for Label, since for the _Create BiMap to work I need to be able to convert it to an AnyValue.

I hope i'm not way off base here and thanks.

chshersh commented 5 years ago

@Nimor111 No problems! Implementing TOML codec for sum types is the challenging part of this issue. Mostly because of poor TOML support of sum types and challenges in supporting sum types in tomland bidirectional codecs. We're still establishing best-practices regarding working with sum types, so bear with me.

The approach with prism works only if you have a single non-nested value in your sum types. LabelRule is a more complicated type. First, let's decide on the TOML format for LabelRule. Since it's a list, you guessed correctly that this should be an array of tables. So let's stick with it for now.

First, let's implement two helper codecs:

labelCodec :: TomlCodec Label
namesCodec :: TomlCodec (Name Label, Name Label)  -- for the old and new names

It shouldn't be difficult since they are product types. Now we would like to combine them into a single codec for LabelRule. And here we need to use the Alternative instance for TomlCodec, and it was implemented to support sum types in the spirit of other libraries. The pseudocode looks like this:

labelRuleCodec :: TomlCodec LabelRule
labelRuleCodec =
    _handleCreate (Toml.table labelCodec "labelRule.create")
    <|> _handleOverride (Toml.table namesCodec "labelRule.override")

Now, _handleCreate and _handleOverride are holes we need to implement. These functions should tell two things:

  1. How to convert underlying codec to LabelRule if it succeeded.
  2. How to convert from LabelRule to the value that underlying codec expects. And if we don't have this value, then do nothing.

So, the missing pieces here are the following functions:

matchCreate :: LabelRule -> Maybe Label
matchOverride :: LabelRule -> Maybe (Name Label, Name Label)

-- I wrote the simplified type here, but it should be generalised in the spirit of `dimap`
disum
    :: (LabelRule -> Maybe t)
    -> (t -> LabelRule)
    -> TomlCodec t
    -> TomlCodec LabelRule

Once you have all this in place, it should be quite easy to implement TOML codec for the LabelRule :slightly_smiling_face: Sorry if this looks complicated :disappointed: Ideally, a combinator like disum should already be in the tomland library along with the appropriate documentation how to write codecs for sum types. But we don't have it (mostly because we haven't needed it and couldn't foresee that we will need it). So that's why it requires a few more steps... For now, you can implement disum directly in this PR and open an issue in the tomland library to add disum later. You can add it to tomland as well if you want; otherwise, I will implement it by myself then:slightly_smiling_face:

Hope this makes things clear! Let me know if you need further clarifications :slightly_smiling_face:

Nimor111 commented 5 years ago

Hello,

Thank you for the awesome pointers, they were very helpful. I'm almost done, but there's one more thing stumping me. :/

Here my implementation for disum:

matchCreate :: LabelRule -> Maybe Label
matchCreate (Create label) = Just label
matchCreate _              = Nothing

matchOverride :: LabelRule -> Maybe (Name Label, Name Label)
matchOverride (Override old new) = Just (old, new)
matchOverride _                  = Nothing

disum
    :: (Functor r, Functor w)
    => (c -> Maybe d) -- ^ Mapper for consumer
    -> (a -> b)       -- ^ Mapper for producer
    -> Codec r w d a  -- ^ Source 'Codec' object
    -> Codec r w c b  -- ^ Target 'Codec' object
disum f g codec = Codec
    { codecRead = g <$> codecRead codec
    , codecWrite = fmap g . codecWrite codec . fromJust . f
    }

This works, but fromJust afaik is definitely a frowned upon (and partial! ) function, but I couldn't find a way to bypass the infinite type d ~ Maybe d I was getting when composing f with codecWrite codec. Any pointers? Thanks again, you guys are a great help!

chshersh commented 5 years ago

Hi @Nimor111, great implementation! You did almost everything correctly, but as you pointed out, we would like to avoid using fromJust. This is indeed a challenging part. I had to spend some time to figure out a satisfiable solution and almost gave up. Nobody did this before, so no surprise there's no ready solution :smile:

I think you need to change disum in the following way:

disum
    :: (Functor r, Alternatve w)
    => (c -> Maybe d) -- ^ Mapper for consumer
    -> (a -> b)       -- ^ Mapper for producer
    -> Codec r w d a  -- ^ Source 'Codec' object
    -> Codec r w c b  -- ^ Target 'Codec' object
disum match ctor codec = Codec
    { codecRead  = ctor <$> codecRead codec
    , codecWrite = \c -> case match c of
        Nothing -> empty
        Just d  -> ctor <$> codecWrite codec d
    }

Let me know if this works :+1:

Nimor111 commented 5 years ago

Thanks, it worked great! I had a similar idea, just had forgotten about Alternative.