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

📝 Add leap approaches #304

Merged
merged 2 commits into from
Jan 7, 2024
Merged

📝 Add leap approaches #304

merged 2 commits into from
Jan 7, 2024

Conversation

jonmcalder
Copy link
Member

I accidentally nuked my WIP on this (uncommitted changes) while switching between branches and rebasing so I had to redo it. Please could you review and check for any issues.

I've opted not to include a performance article for now, but have hinted at benchmarking results for different approaches and the tradeoffs between non-vectorised and vectorised implementations.

@jonmcalder jonmcalder requested a review from colinleach January 7, 2024 17:34
@jonmcalder
Copy link
Member Author

Other questions / consisderations:

I opted for a "conditional-expression" approach like Haskell has rather than "ternary-operator" since R doesn't really have a ternary operator for if () ... else ... though ifelse() is essentially a vectorised ternary operator.

Do you think it is worth including "cheat" approaches for lubridate::leap_year and/or a {lubridate} equivalent for datetime addition?

@colinleach
Copy link
Contributor

I accidentally nuked my WIP on this (uncommitted changes) while switching between branches and rebasing

You have my sympathy, we've all been there.

Do you think it is worth including "cheat" approaches for lubridate::leap_year and/or a {lubridate} equivalent for datetime addition?

Yes, I think it's good to cover all the options in approaches, including things that go beyond what's allowed in Exercism. It's all part of learning about the language, even if student has to go elsewhere to practice. Need to include a warning in the notes, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

[conditional expression][conditional-expression] at line 5 is a broken link, needs a reference at the bottom of the file.

@colinleach
Copy link
Contributor

One broken link in .approaches/introduction.md, otherwise it looks great.

@jonmcalder
Copy link
Member Author

You have my sympathy, we've all been there.

Haha - thanks! 🤗

Yes, I think it's good to cover all the options in approaches, including things that go beyond what's allowed in Exercism. It's all part of learning about the language, even if student has to go elsewhere to practice. Need to include a warning in the notes, though.

Ok sure, will add one or two {lubridate} based approaches in a later PR.

@jonmcalder jonmcalder merged commit 48f6c01 into exercism:main Jan 7, 2024
3 checks passed
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