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

Implement jsx-equals-spacing rule #104

Merged
merged 4 commits into from
Aug 1, 2017

Conversation

bolatovumar
Copy link
Contributor

Issue #103

Ported over the rule from eslint-plugin-react.
Included the exact same tests as in that plugin.

No automatic fixer because I don;t know how it works and never use it myself.
I reckon it would be really easy to add the fixer if someone wanted to do.

@adidahiya
Copy link
Contributor

I strongly recommend adding a fixer here, it should be pretty simple. You just have to add a text replacement for " " -> "".

Can you look into getting CI status reported here?

@bolatovumar
Copy link
Contributor Author

hey @adidahiya, I'm running into some weird issues with the fixer.

Here is what the test output looks like without the fixer applied:
no_fix

It looks like what you would expect to see.

Here is what the test output looks like with the fixer applied:
with_fix

To me it looks like the test should pass yet it fails. Any pointers here?

As per the CircleCI status, I did build my fork through the CircleCI's online interface. It only showed me the master branch, however. Not sure where I'm going wrong here.

@adidahiya
Copy link
Contributor

@bolatovumar can you push your in-progress fixer code here so I can take a look?

@bolatovumar
Copy link
Contributor Author

@adidahiya, sorry, it took me a while. Add my failing WIP fixers above.

The commands I'm running to test the fixers are (from the root of the repo after running npm run build):
./node_modules/.bin/tslint -r ./build/rules/ --test ./test/rules/jsx-equals-spacing/always
./node_modules/.bin/tslint -r ./build/rules/ --test ./test/rules/jsx-equals-spacing/never

In both cases it looks like the console output I'm getting looks something like this:

capture

Hope this is enough detail.

@IllusionMH
Copy link
Contributor

IllusionMH commented Jul 30, 2017

@bolatovumar I've seen something similar when forgot to add empty line at the end of a .fix file.

UPD. Checked changes from PC and now I see that this should be the case: IIRC all lines that mark errors and starts with ~ or [0] etc. should be skipped. All other empty lines should be exactly same as in .lint file.
You can't see diff because line breaks aren't visible and can't be highlighted with color

Copy link
Contributor

@IllusionMH IllusionMH left a comment

Choose a reason for hiding this comment

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

I've marked lines that contain problems.

<App {...props} />

<App foo = {bar} />

Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant line

<App foo = {bar} />

<App foo = {bar} />

Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant line


<App foo = {bar} />

<App foo = {bar} bar = {baz} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably missing empty line after this one.

<App foo={e => bar(e)} />
<App foo={bar} />
<App foo={bar} />
<App foo={bar} bar={baz} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably missing empty line after this one.

<App />
<App foo />
<App foo="bar" />
<App {...props} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing empty line after this one.

<App {...props} />
<App foo={e => bar(e)} />
<App foo={bar} />
<App foo={bar} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing empty line after this one.

@bolatovumar
Copy link
Contributor Author

@IllusionMH, alright, adding and removing line breaks seems to help

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

@adidahiya adidahiya merged commit 7dac374 into palantir:master Aug 1, 2017
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