Skip to content

Clean up Singletons module - #31

Open
mrkgnao wants to merge 5 commits into
k0001:masterfrom
mrkgnao:clean-up-singletons
Open

Clean up Singletons module#31
mrkgnao wants to merge 5 commits into
k0001:masterfrom
mrkgnao:clean-up-singletons

Conversation

@mrkgnao

@mrkgnao mrkgnao commented Jun 18, 2017

Copy link
Copy Markdown

This commit cleans up the horrible TH code in Tisch.Internal.Singletons somewhat.

Comment thread src/lib/Tisch/Internal/Singletons.hs Outdated
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE UndecidableInstances #-}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

FWIW, the reason why I don't align these things vertically is because of what we are seeing here: When one line changes and this change shortens or widens the largest language extension name, then one needs to change all of the lines so that #-} aligns vertically again. The same goes for imports and as, except in imports it's even worse because one also needs to worry about how to format things when the import takes more than one line, something that looks very ugly when things are vertically aligned.

It's a small detail, but it makes maintaining and reviewing code a much pleasurable experience.

@mrkgnao mrkgnao Jun 18, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm sorry, this was from stylish-haskell running automatically on save. I understand the diff problems that aligning code brings. :)

Comment thread src/lib/Tisch/Internal/Singletons.hs Outdated

infixr 3 :&&&

instance SuppressUnusedWarnings (:&&&$) where

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't think we need these SuppressUnusedWarnings things. Can we remove them or do we get some warning? Also, I think you can delete all of the constructors named *KindInference, I don't think we need those.

@mrkgnao mrkgnao Jun 18, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removing them triggers -Wunused-top-binds warnings, which can be disabled module-locally if you'd prefer that. I'll look at the KindInference constructors.

@k0001 k0001 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for doing this! I added some minor comments that would be nice to address, but otherwise this looks way better than what we had 😄

@mrkgnao

mrkgnao commented Jun 18, 2017

Copy link
Copy Markdown
Author

@k0001 it seems I'll be able to cut this module down a lot more.

@k0001

k0001 commented Jun 18, 2017

Copy link
Copy Markdown
Owner

There's something wrong:


src/lib/Tisch/Internal/Table.hs:713:6: error:
    • Couldn't match type ‘Apply
                             (Column_NameSym0 :&&&$$$ Column_PropsSym0) ('Column n w r p h)’
                     with ‘'(n, Column_Props ('Column n w r p h))’
      Expected type: Record
                       (List.Map
                          (Column_NameSym0 :&&&$$$ Column_PropsSym0)
                          ('Column n w r p h : cols))
        Actual type: Record
                       ('(n, Column_Props ('Column n w r p h))
                          : List.Map (Column_NameSym0 :&&&$$$ Column_PropsSym0) cols)
    • In the expression:
        RCons
          (Tagged @n (colProps (Proxy @(Column n w r p h))))
          (rDistributeColProps (Proxy @cols))
      In an equation for ‘rDistributeColProps’:
          rDistributeColProps (_ :: Proxy (Column n w r p h : cols))
            = RCons
                (Tagged @n (colProps (Proxy @(Column n w r p h))))
                (rDistributeColProps (Proxy @cols))
      In the instance declaration for
        ‘RDistributeColProps ('Column n w r p h : cols)’
    • Relevant bindings include
        rDistributeColProps :: Proxy ('Column n w r p h : cols)
                               -> Record
                                    (List.Map
                                       (Column_NameSym0 :&&&$$$ Column_PropsSym0)
                                       ('Column n w r p h : cols))
          (bound at src/lib/Tisch/Internal/Table.hs:712:3)

@mrkgnao

mrkgnao commented Jun 18, 2017

Copy link
Copy Markdown
Author

It seems stack test --file-watch isn't foolproof. Sorry for the noise, I'll try to make a proper commit in some time.

@mrkgnao

mrkgnao commented Jun 19, 2017

Copy link
Copy Markdown
Author

@k0001 everything should work now.

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.

2 participants