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

set default tab width to 8 #957

Merged
merged 1 commit into from Nov 30, 2013
Merged

Conversation

eserte
Copy link
Contributor

@eserte eserte commented Sep 19, 2013

A followup on #951
Now hopefully patched in the right place. http://0:5001/source/GAAS/Encode-Locale-1.03/lib/Encode/Locale.pm looks OK now on my machine with the patch.

@ranguard
Copy link
Member

This is a bit too close to a VIM vs Emacs vs ...

We'd accept an option for each user to customise it for themselves, but otherwise we'll leave it as it is - thanks

@ranguard ranguard closed this Nov 29, 2013
@rwstauner
Copy link
Contributor

I vote to accept this.

On the rare occasion that I actually open a .pm that has literal tab characters
I usually have to readjust my tab size to 8 in order for the display to make sense.
In my experience the files that have tabs use a mixed indentation where less than 8 characters is just spaces
and a level of indentation that rounds to eight ends up as a tab,
so it looks really goofy if the tab-size is set wrong.
Additionally if the file uses tabs exclusively, then it should look ok at 8, too.

The flame wars that accompany tabs only apply to editing files.
If you cat the file (or display it in any other viewer that isn't your editor), it shows as 8 spaces,
therefore, IMO, if you put literal tabs in a file, you should expect that someone else will see them as 8 spaces wide.

@rwstauner rwstauner reopened this Nov 30, 2013
@oalders
Copy link
Member

oalders commented Nov 30, 2013

@rwstauner Do you have an example in mind? I'm just curious as I've never had this problem.

@rwstauner
Copy link
Contributor

Well, the example from #951 is exactly the sort of thing I was talking about:
https://metacpan.org/source/GAAS/Encode-Locale-1.03/lib/Encode/Locale.pm#L23

The if/unless of 24/25 are supposed to be nested, not on top of each other.
The next code lines are eval/undef which should also be nested rather than having the same indent:

Compare tab-width of 4:

23 sub _init {
24     if ($^O eq "MSWin32") {
25     unless ($ENCODING_LOCALE) {
26         # Try to obtain what the Windows ANSI code page is
27         eval {
28         unless (defined &GetACP) {
29             require Win32::API;
30             Win32::API->Import('kernel32', 'int GetACP()');
31         };
32         if (defined &GetACP) {
33             my $cp = GetACP();
34             $ENCODING_LOCALE = "cp$cp" if $cp;
35         }
36         };
37     }

To 8 columns:

23 sub _init {
24     if ($^O eq "MSWin32") {
25         unless ($ENCODING_LOCALE) {
26             # Try to obtain what the Windows ANSI code page is
27             eval {
28                 unless (defined &GetACP) {
29                     require Win32::API;
30                     Win32::API->Import('kernel32', 'int GetACP()');
31                 };
32                 if (defined &GetACP) {
33                     my $cp = GetACP();
34                     $ENCODING_LOCALE = "cp$cp" if $cp;
35                 }
36             };
37         }

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c30dcf8 on eserte:tab-size-8-2nd-try into * on CPAN-API:master*.

@oalders
Copy link
Member

oalders commented Nov 30, 2013

Now I understand. In that case, I'm in favour of this patch. @ranguard?

@ranguard
Copy link
Member

Ahh - ok sure go for it

On 30 Nov 2013, at 03:25, Olaf Alders notifications@github.com wrote:

Now I understand. In that case, I'm in favour of this patch.
@ranguardhttps://github.com/ranguard
?


Reply to this email directly or view it on
GitHubhttps://github.com//pull/957#issuecomment-29544911
.

@oalders
Copy link
Member

oalders commented Nov 30, 2013

Thanks for your persistence @eserte. :)

oalders added a commit that referenced this pull request Nov 30, 2013
@oalders oalders merged commit 693cf2c into metacpan:master Nov 30, 2013
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.

None yet

5 participants