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

cmd/gofmt: incorrect alignment of map keys when Arabic characters are used #16170

Closed
arp242 opened this issue Jun 23, 2016 · 9 comments
Closed

cmd/gofmt: incorrect alignment of map keys when Arabic characters are used #16170

arp242 opened this issue Jun 23, 2016 · 9 comments
Milestone

Comments

@arp242
Copy link

@arp242 arp242 commented Jun 23, 2016

gofmt formats this like so:

var foo = map[string]string{
    "foo":        "bar",
    "foobar":     "foobar",
    "ﺎﻠﻋَﺮَﺒِﻳﺓ": "foo",
    "foo11":    "bar",
    "foobar11": "foobar",
}

However, the expected format would be:

var foo = map[string]string{
    "foo":        "bar",
    "foobar":     "foobar",
    "ﺎﻠﻋَﺮَﺒِﻳﺓ":    "foo",
    "foo11":      "bar",
    "foobar11":   "foobar",
}

It looks like it formats by byte count, rather than character count (did not investigate yet).

Go 1.6.2 on Linux

@quentinmit quentinmit added this to the Go1.8 milestone Jun 23, 2016
@quentinmit
Copy link
Contributor

@quentinmit quentinmit commented Jun 23, 2016

Character count is also insufficient, of course, since characters can have different widths. Still, it seems like we ought to be able to do better.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 23, 2016

gofmt doesn't know about fonts. Without that, it can't do better, and even with fonts knowledge, there's not much it can do since it doesn't control how you view a file (what editor you are using).

gofmt is using text/tabwriter to achieve alignment. This is essentially a duplicate of #8273. There's also the related #12073.

@griesemer griesemer closed this Jun 23, 2016
@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 23, 2016

PS: The issue is not just present for the wider range of Unicode chars, it is really a font issue. There are plenty of people that (would like to) use a proportional font for programming (me included) - after all proportional fonts are used because they increase readability - but gofmt cannot handle those either, even if it's all English.

Since gofmt operates on text files which know nothing about character width on the screen, it must assume that all characters have the same width. At best, it could know which characters are an (integer) multiple of the "unit" width, and then introduce spaces as necessary. However, that would not solve the general problem and unless an editor follows the same formatting, would look just as ugly.

Unfortunate.

@arp242
Copy link
Author

@arp242 arp242 commented Jun 23, 2016

gofmt doesn't know about fonts. Without that, it can't do better, and even with fonts knowledge, there's not much it can do since it doesn't control how you view a file (what editor you are using).

Hm, I would expect the Arabic text in xterm to be monospaced? I don't read Arabic and it's difficult to see, but I have the same problem with Cyrillic characters, which certainly seem monospaced:

var foo = map[string]string{
    "foo": "bar",
    "сииииияяяяя": "z",
    "f1": "foobar",
}

There are plenty of people that (would like to) use a proportional font for programming (me included)

Then gofmt for alignment is useless for you anyway. It already assumes a monospaced font even for plain ol' ascii. Fixing this won't change anything for people using proportional fonts, and it would fix it for people using monospaced fonts. No one loses.

It seems to me that it should be consistent for all characters. Whether or not it's multibyte is not interesting... Having this work only for single-byte characters is going back to the 1980s and C...

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 23, 2016

@Carpetsmoker This is a separate issue. Note the formatting of:

var foo = map[string]string{
    "foo": "bar",
    "сииииияяяяя": "z",
    "f123": "foobar",
}

but:

var foo = map[string]string{
    "foo": "bar",
    "сииииияяяяя": "z",
    "f1234":       "foobar",
}

There is explicit code in gofmt that computes the length ratio between keys on separate lines. If the ratio between two key lengths becomes too large (or too small, depending which way you divide), the alignment of columns is broken. Here the threshold is crossed between f123 and f1234.

The thinking is that we don't want very long such tables where a single long key (possibly far down and not even on the screen when you look at the source) moves all values to the right. It's a heuristic. Your mileage may vary.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 23, 2016

@Carpetsmoker PS: You're right, when using a proportional font, alignment as computed by gofmt is useless. At that point, gofmt only standardizes white space at the beginning of a line. Still better than nothing.

@arp242
Copy link
Author

@arp242 arp242 commented Jun 23, 2016

This is a separate issue.

Ah, okay; thanks. It's not a huge issue anyway, but I've encountered this a few times and sometimes it's annoying that you can't manually correct these little things due to gofmt's "all or nothing approach"...

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 23, 2016

@Carpetsmoker And now that you mentioned it, your original example is following the same heuristics. Note that:

var foo = map[string]string{
    "foo":        "bar",
    "foobar":     "foobar",
    "ﺎﻠﻋَﺮَﺒِﻳﺓ": "foo",
    "foo11":    "bar",
    "foobar11": "foobar",
}

but:

var foo = map[string]string{
    "foo":        "bar",
    "foobar":     "foobar",
    "ﺎﻠﻋَﺮَﺒِﻳﺓ": "foo",
    "foo111":     "bar",
    "foobar11":   "foobar",
}

(GitHub's issue tracker formats a bit differently, but in Sublime the latter is perfectly aligned.) That is, again, here the foo11 crosses the threshold.

I double-checked the implementation of text/tabwriter, and it uses actual rune (Unicode) count to determine the width of text: see updateWidth in https://golang.org/src/text/tabwriter/tabwriter.go?h=updateWidth#L380.

Thus, this is all working as intended.

@arp242
Copy link
Author

@arp242 arp242 commented Jun 24, 2016

Hah, thanks for the clarification @griesemer. As a point of interest, it also seems to be Vim's fault:

2016-06-24-234356_1920x1080_scrot

As you can see, Emacs and Sublime Text work as expected, but Vim doesn't in xterm, lx-terminal, or as gVim...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.