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

Fix: comma-spacing to work with array literals (fixes #1492) #1555

Merged
merged 1 commit into from
Dec 6, 2014

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Dec 6, 2014

No description provided.

@nzakas
Copy link
Member Author

nzakas commented Dec 6, 2014

@btmills if you have a second to give this a once-over, I'll get a bug fix out today

if (isSameLine(previousItemToken, commaToken) &&
isSameLine(commaToken, currentItemToken)) {

if (options.before && options.after) {
Copy link
Member

Choose a reason for hiding this comment

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

There's a whole lot of duplication here that's bugged me for a while. This bug was going to be my excuse to fix it. But I think it can all be reduced to:

if (options.before !== isSpaced(previousItemToken, commaToken)) {
    report(reportItem, getSpaceMsg("before"));
}
if (options.after !== isSpaced(commaToken, currentItemToken)) {
    report(reportItem, getSpaceMsg("after"));
}

If I weren't doing this from my phone, I'd provide an implementation for an appropriate getSpaceMsg function, but it should be just a simple computed property lookup in options using the single srgument as the property string as the condition for a ternary to decide which message (missing, extra) is appropriate. Let me know if that doesn't make sense and I can type that out.

And if you'd rather just keep the existing logic for this fix, I can open a new issue and PR later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, don't want to mess around too much with existing logic. You're welcome to come back later and clean it up however you please - I just want to get people unblocked (I'm also blocked by this on a project).

@btmills
Copy link
Member

btmills commented Dec 6, 2014

I won't be back to my laptop until about 6AM Eastern, meaning I'm doing this from my phone and can't verify yet. I commented about an optional change to the reporting logic that should simplify that section if you want to look into it. The tests for null members look good. Does this happen to handle non-essential parens? Also #1492 was expanded to include both comma-spacing and comma-style. I seem to recall that the logic was similar in both; could this same fix be applied to comma-style, or should #1492 be split back up?

@nzakas
Copy link
Member Author

nzakas commented Dec 6, 2014

I'll take a quick look at comma-style and see how hard it would be to fix, too.

@btmills
Copy link
Member

btmills commented Dec 6, 2014

Hopefully that's an easy fix. Based on a read through and the new tests, I'd say this looks good, even without any additional changes.

@nzakas
Copy link
Member Author

nzakas commented Dec 6, 2014

Oh, but in the future, these sorts of things should be multiple issues. Much easier to track.

@nzakas
Copy link
Member Author

nzakas commented Dec 6, 2014

Okay, not too bad. I think in the future we should consider merging these two rules. There's a ton of duplicate logic.

@btmills
Copy link
Member

btmills commented Dec 6, 2014

👍 LGTM

nzakas added a commit that referenced this pull request Dec 6, 2014
Fix: comma-spacing to work with array literals (fixes #1492)
@nzakas nzakas merged commit 4958587 into master Dec 6, 2014
@nzakas nzakas deleted the issue1492 branch December 6, 2014 22:56
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 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 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants