Skip to content

assign variables: give a reason for the assignment #656

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

Merged
merged 1 commit into from
Jan 13, 2016
Merged

assign variables: give a reason for the assignment #656

merged 1 commit into from
Jan 13, 2016

Conversation

eddiemonge
Copy link
Contributor

I know the examples are contrived but they should at least be best practices in themselves. The variable assignment wasn't necessary. The function could have passed the getter result directly.

https://github.com/airbnb/javascript#13.4

This wasn't necessary:

const name = getName();
this.setFirstName(name);

since it could have been written like this:

this.setFirstName(getName());

This change gives a reason to have it there.

@@ -1136,6 +1139,9 @@ Other Style Guides
}

const name = getName();
if (name.indexOf(' ') > -1) {
name = name.split(' ')[ 0 ];
Copy link
Collaborator

Choose a reason for hiding this comment

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

this isn't good, since it's reassigning a const - it will throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops you are right. Should be lets

Copy link
Collaborator

Choose a reason for hiding this comment

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

(also, there shouldn't be spaces inside brackets: [0])

@eddiemonge
Copy link
Contributor Author

removed the first example as it seemed like a duplicate

@ljharb
Copy link
Collaborator

ljharb commented Jan 13, 2016

LGTM, just needs a fresh rebase :-) thanks!

The variable assignment wasn't necessary. This gives a reason to have it there
@eddiemonge
Copy link
Contributor Author

rebased. not sure why I needed to but did it anyway.

@ljharb
Copy link
Collaborator

ljharb commented Jan 13, 2016

Thanks! If it's on top of latest master I can merge it in locally without a merge commit.

@eddiemonge
Copy link
Contributor Author

If you are doing it locally then its trivial to also do the rebase there as well. Not saying there isn't a reason for the process but it makes it harder to for people trying to help. How often should I be rebasing, every time there is a new commit to master, every day, etc,? A submitter should only rebase if there are conflicts or to squash imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants