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.

Sign up for GitHub

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

Jump to bottom

New: Add consistent and ..-as-needed to object-shorthand (fixes #5438) #5439

Merged
merged 1 commit into from Aug 8, 2016
Merged

New: Add consistent and ..-as-needed to object-shorthand (fixes #5438) #5439

merged 1 commit into from Aug 8, 2016

Conversation

martijndeh
Copy link
Contributor

Please review my implementation for #5438.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @nzakas, @kaicataldo and @IanVS to be potential reviewers

@eslintbot
Copy link

LGTM

@alberto alberto added the do not merge This pull request should not be merged yet label Apr 22, 2016
@alberto
Copy link
Member

alberto commented Apr 22, 2016

Marked as do not merge since the issue is not accepted yet.

@nzakas nzakas removed the cla found label Apr 29, 2016
@nzakas nzakas removed the do not merge This pull request should not be merged yet label May 26, 2016
@nzakas
Copy link
Member

nzakas commented May 26, 2016

@martijndeh if you'd like to rebase, we can start moving this forward.

@alberto alberto mentioned this pull request Jun 14, 2016
Closed
@IanVS
Copy link
Member

IanVS commented Jun 18, 2016

@martijndeh If you're willing to rebase this, I'd love for us to get this merged.

@martijndeh
Copy link
Contributor Author

@IanVS Yes, will do. Thank you.

@kaicataldo
Copy link
Member

kaicataldo commented Jul 4, 2016 •
edited

@martijndeh Pinging one more time since it looks like you did some great work on this! Any chance you'd be willing to rebase? Like @IanVS, I'd love to see this land :)

@martijndeh martijndeh force-pushed the feature/object-shorthand-consistent branch from e341f1f to 204478c Compare July 4, 2016 08:52
@eslintbot
Copy link

LGTM

@martijndeh
Copy link
Contributor Author

I just rebased. Let me know if this needs changing. This is the first time I touched the eslint codebase. :)

nzakas
nzakas reviewed Jul 7, 2016
View reviewed changes
docs/rules/object-shorthand.md
};
```

The following will also *not* warn:
Copy link
Member

Choose a reason for hiding this comment

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

You can combine this with the preceding example. No need to have two code blocks.

@nzakas
Copy link
Member

nzakas commented Jul 7, 2016

Overall looks good, just a bit of refinement in the messaging.

@martijndeh martijndeh force-pushed the feature/object-shorthand-consistent branch from 204478c to 49aff51 Compare July 12, 2016 09:45
@eslintbot
Copy link

LGTM

@martijndeh martijndeh force-pushed the feature/object-shorthand-consistent branch from 49aff51 to d28eaf8 Compare July 12, 2016 09:46
@eslintbot
Copy link

LGTM

@martijndeh
Copy link
Contributor Author

Thanks for the comments. I've replaced the quotes and refined the error messages.

@nzakas
Copy link
Member

nzakas commented Jul 12, 2016

@martijndeh it looks like there are several tests failing now. Can you take a look?

@nzakas
Copy link
Member

nzakas commented Jul 23, 2016

@martijndeh following up, do you intend to finish this up? There are still several tests failing

@martijndeh
Copy link
Contributor Author

@nzakas Yes, I will pick this up later this week.

@martijndeh martijndeh force-pushed the feature/object-shorthand-consistent branch from d28eaf8 to 060bb53 Compare July 29, 2016 11:52
@eslintbot
Copy link

LGTM

@martijndeh
Copy link
Contributor Author

@nzakas Everything should be resolved now. Let me know if there is something else I can do.

@martijndeh martijndeh force-pushed the feature/object-shorthand-consistent branch from 060bb53 to 2835274 Compare July 30, 2016 07:16
@eslintbot
Copy link

LGTM

@martijndeh martijndeh force-pushed the feature/object-shorthand-consistent branch from 2835274 to d259b94 Compare July 30, 2016 07:33
@eslintbot
Copy link

LGTM

@martijndeh
Copy link
Contributor Author

@nzakas I resolved some new conflicts again. I see you're moving to ES6 and noticed the var-to-let refactor. I took the liberty to refactor the object-shorthand rule to use const where possible, even though prefer-const is not enabled (yet). I hope this is ok.

@nzakas
Copy link
Member

nzakas commented Aug 2, 2016

Yup that's fine. Can I trouble you to rebase one more time? We've had a lot of changes lately. (Last time, I promise!)

@martijndeh
Copy link
Contributor Author

Sure no problem. I'll have time later this week.

@martijndeh martijndeh force-pushed the feature/object-shorthand-consistent branch from d259b94 to 775b45a Compare August 8, 2016 09:23
@eslintbot
Copy link

LGTM

@martijndeh
Copy link
Contributor Author

@nzakas I just did a rebase again.

@ilyavolodin
Copy link
Member

LGTM. Thanks!

@ilyavolodin ilyavolodin merged commit 5ef839e into eslint:master Aug 8, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@martijndeh @mention-bot @eslintbot @alberto @nzakas @IanVS @kaicataldo @ilyavolodin @jquerybot