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

Switch column numbers to be 1-based #2284

Closed
nzakas opened this issue Apr 11, 2015 · 12 comments

Comments

Projects
None yet
6 participants
@nzakas
Copy link
Member

commented Apr 11, 2015

Since ESLint has started, we've used 0-based column numbers in our error reporting. This was not an intentional design decision, but rather a side effect of using what Esprima (and now Espree) returned as location position.

Other tools such as JSHint and JSLint are 1-based, and some editors (such as Sublime) expect error messages to come out as 1-based instead of 0-based.

As we are very close to a 1.0.0 release, this is the right time to stop and think if the 0-based columns we've been using are a bug or a feature. I'm leaning more heavily towards "bug" right now, and we could fix this for 1.0.0.

What do others think? Ping @btmills @ilyavolodin @lo1tuma @xjamundx

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Apr 11, 2015

On one hand I think we should be consistent with other linters out there. On the other, we should think very long and hard about making this change. There are a lot of integrations already out there. Some of them are built-in to the editors directly (like WebStorm). So they can only update when the editor updates, which can take a long time. Until then, users will see incorrect reports.

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Apr 11, 2015

Would this change impact the modules which are dependent oneslint?

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Apr 11, 2015

Shouldn't this decision be made by the formatter? 0-based column numbers can be used internally and exposed through the API, and a formatter can choose to display them to a CLI user as 1-based.

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Apr 11, 2015

@michaelficarra I would guess most of the integrations don't use formatters, since they just use API directly and formatter is not required for that.
@gyandeeps Yes, most likely this change would affect anything that depends on ESLint and bypasses formatter.

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2015

@michaelficarra It wouldn't make sense to update every formatter to do this. If 1-based is the right choice, then we shouldn't put the responsibility on formatters to always add 1.

@ilyavolodin Yes, this would be breaking, which is why it would be good to do at 1.0.0. It could break a lot of things, which is why we need to forecast it ahead of time.

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Apr 12, 2015

@nzakas I understand. What I'm questioning is if it's a good idea to make such a disruptive change for no apparent gain other then consistency.

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2015

The gain is that it's a lot less confusing (I'll admit, when I have an error at the 9 column, I tend to do a double take as well). Consistency is important, especially as people switch to ESLint from other tools.

And the disruption is minimal. In the worst case, error reporting is off by one column until people upgrade.

@xjamundx

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2015

I think we should get this right before 1.0!

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Apr 12, 2015

I don't feel strongly either way. Mostly just playing devil's advocate.

@albertosantini

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2015

Before developing Build Next plugin (for SublimeText), I asked in the forum about 0-based column numbers in the error reporting: as @nzakas said, it was Esprima heritage and no option was available.

So I added a preference to the plugin to deal with the issue.

Also in Vim, for instance, you need to adjust the column programmatically.

I gave a look at another tools, as gcc and clang: they are 1-based and I didn't find any discussion about column numbers in the error reporting.

My point is the tools expect 1-based column and I vote for a breaking change.
My second option would be adding a setting to control the behaviour: the default might be also 0-based.

@albertosantini

This comment has been minimized.

Copy link
Contributor

commented May 2, 2015

New kid on the block, Visual Studio Code, is 1-based column number.

https://code.visualstudio.com/Docs/tasks

@nzakas nzakas added this to the v1.0.0 milestone May 9, 2015

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2015

Working on this.

@nzakas nzakas closed this in 091d6bc Jul 2, 2015

nzakas added a commit that referenced this issue Jul 2, 2015

Merge pull request #2908 from eslint/issue2284
Breaking: Switch to 1-based columns (fixes #2284)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.