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

English - Basic Modules #28

Merged
merged 12 commits into from
Sep 8, 2021
Merged

English - Basic Modules #28

merged 12 commits into from
Sep 8, 2021

Conversation

JonathanLorimer
Copy link
Contributor

Closes #26

@TrueBoxGuy
Copy link
Contributor

I added some comments related to grammar (just for the start of the article), but I'll probably have to learn more about modules myself to make a more substantive review.

@TrueBoxGuy
Copy link
Contributor

image
Here's a page screenshot for other reviewers.

Copy link
Contributor

@TrueBoxGuy TrueBoxGuy left a comment

Choose a reason for hiding this comment

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

This lesson looks very good to me. The only issue is really with introducing types and typeclasses, which may confuse the reader this early on.

en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Show resolved Hide resolved
en/lessons/basics/modules.md Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
Copy link
Contributor

@TrueBoxGuy TrueBoxGuy left a comment

Choose a reason for hiding this comment

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

From my perspective, this lesson does everything it needs to.

Sorry for not picking up on some other grammar issues.

Comment on lines 77 to 78
only the type `Identity` (and its constructor) and `id` are exported, `compose`
is completely isolated to the local module scope.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a comma splice here.

Comment on lines 9 to 10
a namespace that allow us to aggregate common pieces of functionality and avoid
naming collisions. However, modules are use to build up more complex constructs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
a namespace that allow us to aggregate common pieces of functionality and avoid
naming collisions. However, modules are use to build up more complex constructs
a namespace that allows us to aggregate common pieces of functionality and avoid
naming collisions. However, modules are used to build up more complex constructs

It required three proof readings to spot this😅.

Copy link
Contributor

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

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

A couple of changes, one FYI, and we should link to https://wiki.haskell.org/Import at the bottom

en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
Copy link
Contributor

@TrueBoxGuy TrueBoxGuy left a comment

Choose a reason for hiding this comment

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

I didn't have much to comment on.

en/lessons/basics/modules.md Outdated Show resolved Hide resolved
Comment on lines 115 to 118
There is only one piece of syntax that ever comes before module syntax (aside
from a haddock module declaration comment, haddock is discussed in a separate
section); that is language extensions. We won't go over what they do here, but
just so you are familiar this is what they look like.
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the initial issue I opened involved specifying the full structure of a file, but I feel like going into the existence of Haddock may be a bit too clunky here.

If the phrasing "only one piece of syntax" were removed, peculiarities (like Haddock and other pragmas) wouldn't need to be mentioned.

I'm probably still in favour of mentioning language extensions, because they are used even in the basics page of the site, and it would be useful for the reader to understand how to enable them in other contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I made changes removed the unnecessary bit about module syntax. I went in more depth about language extensions and overloaded strings in particular to try and assuage any confusion, I would be interested to hear if you think it adds more or less confusion.

Comment on lines 135 to 136
Modules can be listed as a dependency within other modules. It is convention to
have these declarations at the top of a file, below the module declaration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it necessary to put imports before top level declarations because of the parser?

(Sorry I forgot to put this in the review)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, good call, bad wording on my part.

Copy link
Contributor

@TrueBoxGuy TrueBoxGuy left a comment

Choose a reason for hiding this comment

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

I feel like your explanation of language extensions has appropriate length and covers the right things.

Could you possibly run British English spell check on the document before merging?

en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
en/lessons/basics/modules.md Outdated Show resolved Hide resolved
Copy link
Contributor

@TrueBoxGuy TrueBoxGuy left a comment

Choose a reason for hiding this comment

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

Everything looks fine to me.

@joelmccracken
Copy link
Contributor

Rebasing this PR onto development fixed the issue for me locally. I'd say try that.

@JonathanLorimer
Copy link
Contributor Author

@joelmccracken I was really hoping that would just work

@joelmccracken
Copy link
Contributor

drat, sorry. I was really hoping, too.

this version of the branch is still fine locally, so I cloned it and am gonna investigate further.

@joelmccracken
Copy link
Contributor

Well, i have the problem occurring on my fork of this project, so, progress?

@JonathanLorimer JonathanLorimer merged commit 4df5d05 into development Sep 8, 2021
@JonathanLorimer JonathanLorimer deleted the en/basic-modules branch September 8, 2021 18:19
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.

English - Basics/Modules
4 participants