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

First PR #281

Closed
wants to merge 6 commits into from
Closed

First PR #281

wants to merge 6 commits into from

Conversation

yTakkar
Copy link
Contributor

@yTakkar yTakkar commented Mar 6, 2018

Introduced ES6 syntax to JS files.
And some minor changes in installation command and other projects' list of README.md.

@zenflow
Copy link

zenflow commented Mar 6, 2018

This module currently supports node version 4, which does not support ES6 syntax (without the --harmony flag) so I think this can't be merged right now.

But node version 4 reaches End-Of-Life on 2018-04-30. So maybe then we can stop supporting node version 4 too, and switch to ES6 syntax.

@zenflow
Copy link

zenflow commented Mar 6, 2018

@yTakkar I suggest breaking up this PR into separate PRs

@yTakkar
Copy link
Contributor Author

yTakkar commented Mar 6, 2018

@zenflow I think you're right, I should break up this PR into separate PRs

@zenflow
Copy link

zenflow commented Mar 6, 2018

@yTakkar Maybe open 3 fresh PRs rather than undoing changes here, so it's easier to review

@maxbeatty
Copy link
Contributor

👋 thanks for contributing! We've found more success in the past when someone creates an issue with a suggestion for a change; a majority of people agree on it, and then a Pull Request is created.

@zenflow
Copy link

zenflow commented Mar 6, 2018

@maxbeatty Yeah, that approach is usually better, but for simple changes like f1cd386 it's easier and faster and makes more sense to suggest the change and discuss it in a PR, right?

@zenflow
Copy link

zenflow commented Mar 6, 2018

I think the reason it's better to discuss non-trivial changes in an issue first is because it could be rejected and then your time coding the changes was wasted.

@yTakkar
Copy link
Contributor Author

yTakkar commented Mar 6, 2018

But what if someone suggests any change and there's no issue about that change??
I mean PRs will never be accepted/merged if it doesn't have any related issue??

@yTakkar
Copy link
Contributor Author

yTakkar commented Mar 6, 2018

I'll suggest merging it, after all it's a transition of code to es6 onyl!!

@maxbeatty
Copy link
Contributor

To minimize wasted effort, make sure your change is wanted before spending time on it. Issues are a good way to do that. Well-written Pull Request descriptions are another way.

Today, I do not see enough benefit to accept these changes. If in the future new syntax results in a performance gain, safer execution, or similar improvement, I would be open to updating it.

@maxbeatty maxbeatty closed this Mar 6, 2018
@zenflow
Copy link

zenflow commented Mar 6, 2018

I mean PRs will never be accepted/merged if it doesn't have any related issue??

I think there are exceptions. See my comment #281 (comment). I don't know if @maxbeatty agrees because he didn't reply to me.

@yTakkar yTakkar deleted the first-branch branch March 12, 2018 10:22
@yTakkar yTakkar restored the first-branch branch March 12, 2018 10:26
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.

3 participants