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

Add fix option to comma-spacing (fixes #4232) #4233

Merged
merged 1 commit into from
Oct 27, 2015
Merged

Conversation

netei
Copy link

@netei netei commented Oct 23, 2015

I'm not sure how to handle the last cases with comments :

        {
            code: "myfunc(404, true/* bla bla bla */ , 'hello');",
            output: "myfunc(404, true/* bla bla bla */, 'hello');",
            errors: [
                {
                    message: "There should be no space before ','.",
                    type: "Punctuator"
                }
            ]
        },
        {
            code: "myfunc(404, true/* bla bla bla */ /* hi */, 'hello');",
            output: "myfunc(404, true/* bla bla bla *//* hi */, 'hello');",
            errors: [
                {
                    message: "There should be no space before ','.",
                    type: "Punctuator"
                }
            ]
        }

doesn't work, it is removing the comments.

@netei netei force-pushed the issue4232 branch 2 times, most recently from ccfb1bf to 4486df4 Compare October 23, 2015 07:23
@alberto alberto added the do not merge This pull request should not be merged yet label Oct 23, 2015
@alberto
Copy link
Member

alberto commented Oct 23, 2015

You should add the expected output of those tests to the PR, even if it makes it fail (that's the point of the tests afterall).

@netei
Copy link
Author

netei commented Oct 23, 2015

I've added the failing tests

@alberto alberto removed the do not merge This pull request should not be merged yet label Oct 23, 2015
@nzakas
Copy link
Member

nzakas commented Oct 23, 2015

Then work to fix the tests :)

@netei
Copy link
Author

netei commented Oct 26, 2015

I finally found a solution. The tests pass now

@nzakas
Copy link
Member

nzakas commented Oct 26, 2015

Change looks good. Can you please sign our CLA? http://eslint.org/cla

@netei
Copy link
Author

netei commented Oct 27, 2015

Signed

@nzakas
Copy link
Member

nzakas commented Oct 27, 2015

Thanks!

nzakas added a commit that referenced this pull request Oct 27, 2015
Add fix option to comma-spacing (fixes #4232)
@nzakas nzakas merged commit 34e8700 into eslint:master Oct 27, 2015
@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.

None yet

3 participants