-
Notifications
You must be signed in to change notification settings - Fork 71
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 map example, and couple of TODOs #154
Conversation
@@ -3,3 +3,4 @@ tmp/ | |||
/npm-debug.log | |||
.directory | |||
.kateproject | |||
.idea/ |
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 should live in your global .gitignore.
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 :D but I like to commit project files in my other projects.
- I was wondering if
.kateproject
can live here, what's the harm in keeping.idea
/ ?
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 was thinking the same, (either remove kateproject or add idea) as long as we standardise the /
usage.
Unless a core member is using Kate, then that would bump it's priority.
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.
We should remove .directory
and .kateproject
as well, unless they're actually generated by this project.
console.log(html) | ||
|
||
``` | ||
Above code is too verbose and we are prone to making mistakes. Ramda provides a `map` function to achieve |
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.
To my mind, the problem with a for
loop is it mixes together the iteration and the transformation. The map
version abstracts away the iteration, so all you have to do is write the function that transforms each element.
This leaves aside the fact that map
is for more than just lists, of course ...
R.map((lib)=>"<li>"+lib+"</li>", libs) | ||
``` | ||
|
||
Much more concise. We could refactor this code further by writing a reusable wrapTag function. |
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.
Concision is not a goal in itself; clarity should be. This is clearer because we don't need to know which element we are transforming (e.g. libs[i]
)
I think this is a great start! I agree with the comments from @buzzdecafe and I have a few more to add myself (tomorrow; it's bedtime for me.) But I like where this is going! |
It may be pedantic, but would it be worthwhile changing the bold items to headings? This may require rewording the opening sentence and the headings. |
I'd really like to see the manual retain a consistent style throughout. One thing this section does very different from the few others written so far is to make the headers integral to the narrative, and deeply interconnected. So I would prefer to see a little introductory matter with this in plain text:
followed by sections simply headed "Transforming", "Filtering", etc. |
console.log(html) | ||
|
||
``` | ||
Above code is too verbose and we are prone to making mistakes. Ramda provides a `map` function to achieve |
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.
We should link to the functions on first usage:
... Ramda provides a [`map`][ma] function to...
[ma]: http://ramdajs.com/docs/#map
One other stylistic point. The two fairly completed sections, Small Functions and Immutability try to intersperse explanatory text and code, but every bit of code has some introduction. Rather than an explanation after you see it, you have a description of what's coming before you do. I think it would be useful to stick to that throughout. |
Jigar Gosar On 2 November 2016 at 17:51, Scott Sauyet [email protected] wrote:
|
enjoy -- no need to rush |
@jigargosar: I've also gotten back to working on this. I'll submit a PR for another section today or tomorrow. But this has already been very slow. A few days or a week is nothing at all! 😄 |
@CrossEye Thanks Scott, looking forward to your next article.
|
* Using examples from https://github.com/CrossEye/FuncProgTalk * Added introductory text. * Code examples are preceded by explanatory text. * Changed emphasis from 'Conciseness' to 'Clarity' * Added link to methods on first usage.
Hopefully I have addressed all the comments in the last commit. |
@davidchambers I would like to understand the reasoning behind removing
|
@jigargosar how many and which configuration files should we ignore? Ideally each person would be ignoring files/folders created by their editor, unless they are dependencies of this project. |
What @MattMS said. :) Let's assume I contribute to 100 projects, and use a text editor which creates a "hidden" file in a directory whenever I edit a file in that directory. I could either:
|
@davidchambers Thanks for the explanation. Now I understand the motivation.
|
This pull request is quite old, @jigargosar. Shall we close it, or are you interested in completing it? |
Looking for feedback. To avoid continuing on the wrong path.