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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

key-spacing + align: 'value' + jsx objects becomes ugly sometimes #11414

Closed
a-x- opened this issue Feb 19, 2019 · 7 comments 路 May be fixed by rsumner868/librealsense2.1#1, rsumner868/librealsense#4, nl253/BlogUI#3, O330oei/node#4 or O330oei/node#11

Comments

@a-x-
Copy link

@a-x- a-x- commented Feb 19, 2019

screenshot 2019-02-19 at 18 51 23

image

馃憖 Look at the repo with minimal reproducible example (dependencies, config, source): https://github.com/a-x-/eslint-align-check






environment

  • ESLint Version: latest (5.13.0)
  • Node Version: latest (11.9.0)
  • npm Version: latest (6.8.0)

parser: Babel-ESLint

minimal configuration: (full config has no sense never : )

{
  "plugins": [
    "react",
  ],
  "rules": {
    "key-spacing": ["warn", {
      "multiLine": {"align": "value"},
    }],
    // some related a bit rules
    "object-curly-newline": ["warn", { "multiline": true, "minProperties": 5 }],
    "object-property-newline": ["warn", {"allowAllPropertiesOnSameLine": true }],
    "object-curly-spacing": ["warn", "always", {
      "arraysInObjects": false,
      "objectsInObjects": false,
    }],
    // ...
  },
}

source and command

source before eslint --fix ran
function Component () {
      return <div>
        <span style={{ display: 'inline-block', marginRight: '10px' }}>
          <Fa
            icon={ d.icon }
            style={{ marginRight: '4px', color: '#0C090A', opacity: '0.6', fontSize: '18px', verticalAlign: 'middle' }}
          />
          <span style={{ verticalAlign: 'bottom' }}>{ d.title }</span>
        </span>
        <span>
          <Fa
            icon="location-arrow"
            style={{ marginRight: '4px', color: '#0C090A', opacity: '0.6', fontSize: '18px', verticalAlign: 'middle' }}
          />
          <span style={{ verticalAlign: 'bottom' }}>{ this.state.info?.device.app_version }</span>
          <br />
          <span style={{ color: '#999999' }} title="袣芯谐写邪 锌芯褋谢械写薪懈泄 褉邪蟹 写械谢邪谢 褔褌芯-褌芯 胁 锌褉懈谢芯卸械薪懈懈">
            { this.state.info?.device.last_request_at }
          </span>
        </span>
      </div>;
}
eslint --fix file.jsx

Expected: don't re-align objects in jsx curlies.

PR? I'm not sure, maybe I can.

@a-x- a-x- added bug triage labels Feb 19, 2019
@aladdin-add aladdin-add added rule evaluating and removed triage labels Feb 19, 2019
@aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Feb 20, 2019

thanks for the issue, I was able to repro it!

@aladdin-add aladdin-add added accepted and removed evaluating labels Feb 20, 2019
@eslint eslint bot closed this May 21, 2019
@eslint eslint bot added the auto closed label May 21, 2019
@eslint
Copy link
Contributor

@eslint eslint bot commented May 21, 2019

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Aug 15, 2019

reopen as it is happening in latest eslint and default parser: I've created a mini demo

@yeonjuan
Copy link
Member

@yeonjuan yeonjuan commented Oct 22, 2019

@aladdin-add
For fixing this issue,
I thought that the multiple properties on a single line should be treated as a single line. And fix it. (#12472) . Am I missing something?

({
   a: 1, b: 3, c:4, // single line.
});
kaicataldo added a commit that referenced this issue Nov 25, 2019
kaicataldo added a commit that referenced this issue Nov 25, 2019
Revert "Update: Fix uglified object align in key-spacing (fixes #11414) (#12472)"

This reverts commit 6503cb8.
kaicataldo added a commit that referenced this issue Nov 25, 2019
Revert "Update: Fix uglified object align in key-spacing (fixes #11414) (#12472)"

This reverts commit 6503cb8.
kaicataldo added a commit that referenced this issue Nov 25, 2019
Revert "Update: Fix uglified object align in key-spacing (fixes #11414) (#12472)"

This reverts commit 6503cb8.
btmills added a commit that referenced this issue Nov 25, 2019
Revert "Update: Fix uglified object align in key-spacing (fixes #11414) (#12472)"

This reverts commit 6503cb8.
@kaicataldo kaicataldo reopened this Nov 25, 2019
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 25, 2019

We decided to revert the PR that closed this due to a regression. @yeonjuan We would love to work with you on finding an alternate solution, if you'd like still to work on this!

@yeonjuan
Copy link
Member

@yeonjuan yeonjuan commented Nov 25, 2019

@kaicataldo
I still want to work on this :).
This is an alternative solution what I thought.

const obj = {
    key1: "value1", key2: "value2",  // (1)
}

The line (1) should be checked by a multiline option(multiLine.beforeColon, multiLine.afterColon) because the obj should be considered as a multi-line object.

But the multiLine.align is for vertical aligning. So what about just ignoring a multiLine.alignoption in this case(multiple props on the same line)?

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 25, 2019

The configuration options for this rule are really confusing 馃槵

I think your proposal makes sense and we should ignore aligning a line when it contains multiple properties.

So these would be correct:

/*eslint "key-spacing": [2, {
    "singleLine": {
        "beforeColon": false,
        "afterColon": true
    },
    "multiLine": {
        "beforeColon": true,
        "afterColon": true,
        "align": "colon"
    }
}]*/
var obj = { one: 1, "two": 2, three: 3 };
var obj2 = {
    "two" : 2,
    three : 3
};
var obj2 = {
    one : 1, "two" : 2, three : 3 
};

Also curious what other team members think.

yeonjuan added a commit to yeonjuan/eslint that referenced this issue Dec 6, 2019
yeonjuan added a commit to yeonjuan/eslint that referenced this issue Dec 7, 2019
@kaicataldo kaicataldo added the autofix label Dec 10, 2019
btmills added a commit that referenced this issue Dec 20, 2019
* Fix: ignore aligning single line in key-spacing (fixes #11414)

* add test cases

* add tests for protect against regressions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.