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

AuthMethods + Template Haskell experiment #1918

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

infomiho
Copy link
Contributor

@infomiho infomiho commented Mar 23, 2024

I was thinking how can we make adding a new auth method a more type safe process. Since we are kinda loosely defining them by adding a new key in the AuthMethods record, we don't really have a union data type over which we can force exhaustive pattern matches.

I've played a bit with Template Haskell to try to change that.

Signed-off-by: Mihovil Ilakovac <[email protected]>
Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Interesting experiment @infomiho , and good job on learning more about TH in practice!

Looking at the PR, it seems to me like the conclusion is that TH brought more complexity then it took away. How are you thinking about it?

AUthMethods part of PR looks reasonable to me, although I would have to see it without TH to make proper review. But TH changes I think you can drop. It would be interesting to understand better what you tried to solve with them, and see if we can achieve that in some other way -> maybe by organizing some code differently, or by using typeclasses, or something like that. Maybe even some TH again, but I think it will be much simpler, if it makes sense at all.


userSignupFieldsForExternalAuth :: ExternalAuthConfig -> Maybe ExtImport
userSignupFieldsForExternalAuth = userSignupFields
generateIsAuthMethodEnabled :: Q [Dec]
Copy link
Member

Choose a reason for hiding this comment

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

Since TH can be hard to undrsatnd, and here it certainly is a bit ahrd to read, it would be good if you could document here, in a comment above it, what does this TH generate.

where

import Data.Data (Data)
import Language.Haskell.TH
Copy link
Member

Choose a reason for hiding this comment

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

Unqalified, unlisted import -> not a good practice!
I would suggest importing it qualified as TH.

Comment on lines +92 to +99
userSignupFieldsForEmailAuth :: EmailAuthConfig -> Maybe ExtImport
userSignupFieldsForEmailAuth = userSignupFields

userSignupFieldsForUsernameAuth :: UsernameAndPasswordConfig -> Maybe ExtImport
userSignupFieldsForUsernameAuth = userSignupFields

userSignupFieldsForExternalAuth :: ExternalAuthConfig -> Maybe ExtImport
userSignupFieldsForExternalAuth = userSignupFields
Copy link
Member

Choose a reason for hiding this comment

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

What you can do, if you want is, use typeclass for something like this, which has a method userSignupFields, and each of these types will implement it. That might be a bit more elegant, depending on how you use it though, what you really need.

configType GitHub = ''ExternalAuthConfig

-- Generate the AuthMethods data type
-- data AuthMethods = AuthMethods
Copy link
Member

Choose a reason for hiding this comment

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

Seems to me like you ended up defining this relationship anyway above! Becauwe you defined it via configType at the end, only to then generate AuthMethods from that. So did you really gain anything?

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