-
-
Notifications
You must be signed in to change notification settings - Fork 359
[Haskell] Computus #679
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
[Haskell] Computus #679
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
computus mode year = | ||
let | ||
-- Year's position on the 19 year metonic cycle | ||
a = year `mod` 19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type signatures are recomended for local bindings. They help you think and avoid crazy type errors on basic mistakes (as ghc will just try make it work somehow).
So consider adding: a :: Int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that achieve anything? The signature of mod
is mod :: Integral a => a -> a -> a
and since year
is an Int
, a
cannot possibly be anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make a mistake on writing implementations, having your code blotted with typesignatures will avoid crazy ghc deductions. It's more defensive. Typesignatures rarely do anything in haskell (1). It'sHindley–Milner all the way down.
(1): Bar some major exceptions from langauge extensions, such as datakinds, GADTs etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it would hinder readability I think. And since all the local bindings derive from year
, it would do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind going either way here. It looks like this conversation is the only one holding this PR back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see the confusion, how about this:
a :: Int
a = year `mod` 19
let blocks can have same typesignatures as functions. In fact, you can put functions in let blocks!
Don't worry about being stubborn, this is constructive in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this conversation is still going, so let me know when both of you are happy with the code!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I"m not getting a reply so yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did reply, on May 24, on another comment. I tried what you suggested (with a twist).
EDIT: nope, that comment was for something else. I pushed a commit but did not comment. Sorry about that.
|
||
-- Finding the next Sunday | ||
Easter -> | ||
let |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a knitpick: since haskell is lazy, there is no need for doing 2 of these let bindings, in fact I'd just throw all these variables in a big where block and put the logic (case mode of ... if then) at the top. This avoids the nesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose this way because it helps to visually understand that Easter
involves more steps than Servois
. I can change it if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it also looks more like the other code like this. It's just not what I'm used too seeing, so I'll leave it up to your discretion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you, it also felt strange to me, this algorithm is strange :)
I tried another way, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to merge, if you guys are. Thanks for the code and review!
computus mode year = | ||
let | ||
-- Year's position on the 19 year metonic cycle | ||
a = year `mod` 19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind going either way here. It looks like this conversation is the only one holding this PR back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because all discussions here seem to be wrapped up, I am happy to merge.
Thanks again for the contribution!
Added the Haskell implementation of the algorithm. It's been a while :)
Super straightforward translation of the Julia code.