-
-
Notifications
You must be signed in to change notification settings - Fork 339
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 comments to indicate where imports can be added #53
Add comments to indicate where imports can be added #53
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.
Thanks for improving the docs! I have only one suggestion 🙂
src/Lecture3.hs
Outdated
{- | If you need to import libraries, do it after this line ... -} | ||
|
||
{- | ... and before this line. Otherwise the test suite might fail -} |
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 think we need to have such a comment only in this file. Because only this module has the $setup
part that needs to happen after imports. The rest are fine 🙂
Also, I've added some arrows to attract more attention 👀
{- | If you need to import libraries, do it after this line ... -} | |
{- | ... and before this line. Otherwise the test suite might fail -} | |
{- VVV If you need to import libraries, do it after this line ... VVV -} | |
{- ^^^ ... and before this line. Otherwise, the test suite might fail ^^^ -} |
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.
My reasoning for adding them to all files was:
- Instructional: show people where it is customary to add imports.
- Non-spoilery: I think
Lecture3.hs
is the first set of exercises where importing a library might make it easier, but no need to give that away?
Like the arrows ;-)
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.
@bulters Makes sense 🙂 I agree with that. In that case, if you could add arrows to all newly introduced sections, happy to merge 👍🏻
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.
Done.
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.
Looks great! 👍🏻
…s-2022#53) * Add comments to indicate where imports can be added * Add arrows to indicate where imports can be added Co-authored-by: Dmitrii Kovanikov <kovanikov@gmail.com>
This is to fix issue #52