Skip to content

Add some syntax sugar #143

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

Closed
wants to merge 1 commit into from
Closed

Add some syntax sugar #143

wants to merge 1 commit into from

Conversation

evenstensberg
Copy link

This PR will add an comment with an discription of what spawn do, as
well as an removed console.log in order to make use of string template
in node.

Not sure if this is needed, but why not 💙

This PR will add an comment with an discription of what spawn do, as
well as an removed console.log in order to make use of string template
in node.
@ghost ghost added the CLA Signed label Jul 23, 2016
@evenstensberg
Copy link
Author

This is greatly written by the way. Thumbs up to Christopher, Dan and the rest working on this.

@gaearon
Copy link
Contributor

gaearon commented Jul 23, 2016

Hi, thanks! I don’t think we’ll take this.

We use spawn pretty extensively in the codebase so it doesn’t make sense to me to document what it does in just one place. One would need to understand spawn anyway to contribute to this project, so I feel like documenting this is a bit out of scope.

Similarly, we would rather avoid taking purely stylistic changes. They clutter the changelog, and this code changes fast anyway. Also, I’m not sure I’m seeing the benefit of proposed over existing form.

@gaearon gaearon closed this Jul 23, 2016
@evenstensberg
Copy link
Author

No problem, I get it :) You'll have one console line instead of two, which makes a bit more sense, as you are making the code a bit more efficient in that matter. However, the changes to not have a very big perf gain, so I understand.

As with the commenting, I like that the .sh has been fairly commented, so people that want to contribute know how the bash script works. It was a nice move. It makes it easier to understand the code if you see something you've not encountered before, as well as getting knowledge of how it works, before eventually making a PR that's related to source code.

Also, if it's not a too big of a deal,mind answering me on Twitter, so I can ask a few questions about how you're making a temp directory.

kalekseev pushed a commit to kalekseev/create-react-app that referenced this pull request Sep 11, 2017
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants