Reformat std #8408

Aurel300 · 2019-06-11T12:45:47Z

Closes #7404.

added Haxe Foundation preamble to all classes
reformatted all classes in std with haxelib run formatter
- default (no) configuration
- very few manual changes, see: Doc formatting upset by conditional compilation HaxeCheckstyle/haxe-formatter#499

I didn't touch the non _std classes in std/flash, nor the auto-generated externs in std/js, since those should be addressed separately. Some interesting finds that should maybe be addressed at some point:

std/cpp/zip/Compress.hx#L24 - #if (haxe_ver < 9000.0) - in all cpp.zip and neko.zip classes
https://github.com/HaxeFoundation/haxe/blob/development/std/cs/_std/haxe/ds/WeakMap.hx2 - should this file be removed completely or should it be .hx?
target-specific implementations should not have docs from the std externs

Future goals:

run formatter as a git pre-commit hook
ensure HTML externs are generated consistently with the format
ensure Flash externs (--gen-hx-classes) are generated consistently with the format
ensure macro Printer is consistent with the format

kLabz · 2019-06-11T12:52:38Z

WeakMap has been renamed by 67d74cd on Mar 15, 2015 with this message:

Disable WeakMap for now since it's broken (hxgeneric error)

So I guess it's fine to remove it now?

…reformat

ncannasse · 2019-06-12T07:13:10Z

std/Any.hx

-	@:noCompletion @:from extern inline static function __cast<T>(value:T):Any return cast value;
-	@:noCompletion extern inline function toString():String return Std.string(this);
+	@:noCompletion @:to extern inline function __promote<T>():T
+		return this;


I think this should be kept as a single line by the formatter.

ncannasse · 2019-06-12T07:15:33Z

std/Date.hx

+
+	#if flash
+	private static function __init__():Void
+		untyped {


same here, untyped should be kept on declaration line

Agree, there's an open issue for this: HaxeCheckstyle/haxe-formatter#362

std/Lambda.hx

ncannasse · 2019-06-12T07:19:57Z

std/StdTypes.hx

-@:coreType abstract Void { }
+#if jvm
+@:runtimeValue
+#end


I think these should also be kept as single-line

Probably the same as HaxeCheckstyle/haxe-formatter#332.

std/StringTools.hx

ncannasse · 2019-06-12T07:23:48Z

std/UInt.hx

-	@:op(A<=B) private static function ltef2(lhs:Float, rhs:UInt):Bool;
+abstract UInt to Int from Int {
+	@:commutative @:op(A + B) private static function addI(lhs:UInt, rhs:Int):UInt;
+


No newlines when there is consecutive methods with no body seems like a good rule

Agreed. That is normally the logic for externs / interfaces / etc, I think the formatter doesn't account for body-less functions being a thing in externs too yet.

ncannasse · 2019-06-12T07:26:14Z

std/cpp/Int32.hx

@@ -19,6 +19,7 @@
 * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 * DEALINGS IN THE SOFTWARE.
 */
- package cpp;
+
+package cpp;


Why ? just after the initial (c) block seems fine to me.

Agreed, there's an open issue for this here: HaxeCheckstyle/haxe-formatter#310

So if I understand correctly, this is configurable, but the default is currently set to 1 line?

Sort of... but changing the setting also removes empty lines between multiline comments (HaxeCheckstyle/haxe-formatter#292). I'd want both, which isn't possible yet.

std/cpp/Lib.hx

ncannasse · 2019-06-12T07:44:25Z

I have added a few comments.
There a several important things I would like to discuss:

Conditional blocks indentation

Original:

#if hl
    return bla();
#elseif sys
    return foo();
#else
    #if haxe4
        return a();
    #else
        return b();
    #end
#end

Reformated:

#if hl
return bla();
#elseif sys
return foo();
#else
#if haxe4
return a();
#else
return b();
#end
#end

I think that we should enforce indentation when into a "block-level" #if

Also, I think we should be able to keep the single line #if haxe4 a() #else b() #end - it seems to be multlined by formater for types metadata.

Switch cases indentation

That's more of a controversial one, that will most likely hurts some's habits.
I would like the official indentation for case to be on the same level as switch. This will honor the roots of Haxe pattern matching which is OCaml match where patterns are not indented. This also makes multiple inner switches to be more readable because there will be less indentation involved.

switch (e) {
case Value(x):
    return x;
case Add(x,y):
    return eval(x) + eval(y);
case Throw(v);
    switch( v ) {
    case String(str): throw str;
    default:
    }
}

Instead of:

switch (e) {
    case Value(x):
        return x;
    case Add(x,y):
        return eval(x) + eval(y);
    case Throw(v);
        switch( v ) {
            case String(str): throw str;
            default:
        }
}

Single line should be kept

Some single line code should not be transformed into multiple line, for instance the following:

if( x == 0 ) continue;

Should not be transformed into:

if( x == 0 )
    continue;

And the following:

[for( x in 0...MAX ) for( y in 0...MAX ) x * y];

Should not be transformed into:

[
    for( x in 0...MAX )
        for( y in 0...MAX )
            x * y
]

Gama11 · 2019-06-12T08:36:58Z

Conditional blocks indentation

This style of formatting can be achieved with the following setting:

{
	"indentation": {
		"conditionalPolicy": "alignedIncrease"
	}
}

However, it does not differentiate between block-level conditionals and others. I guess that would make it more bearable. We might need an additional, separate conditionalPolicyBlock setting @AlexHaxe?

Still, a bit surprising since it's the opposite of your switch-case indentation preference, where you want to avoid indentation. ;) I was going to say the current formatter default is more in line with the current style in std, but it actually seems to be all over the place and uses at least three different styles for conditional indentation at block level.

Switch cases indentation

I can definitely see the advantages of this one (recently opened an issue for the formatter to support it, actually: HaxeCheckstyle/haxe-formatter#478). However, I think it's quite rare for code in std / for Haxe code in general to be formatted like this. I have a feeling @Simn might not like it for instance?

Single line should be kept

I think the formatter already supports this one through a "keep" setting, but I'm not sure if it's a great idea. Everywhere such a "keep" setting is used, the resulting code will be less consistent since it depends on the original formatting of the file instead of enforcing one style.

Gama11 · 2019-06-12T08:38:42Z

Oh, and in general, I'd really like the default settings of the formatter to be the same as the ones used in std. So this is also a discussion about the defaults of the formatter, not just the hxformat.json config of std. But it looks like we're not too far off with the current defaults.

RealyUniqueName · 2019-06-12T09:10:28Z

I would like the official indentation for case to be on the same level as switch. This will honor the roots of Haxe pattern matching which is OCaml match where patterns are not indented. This also makes multiple inner switches to be more readable because there will be less indentation involved.
switch (e) {
case Value(x):
    return x;
case Add(x,y):
    return eval(x) + eval(y);
case Throw(v);
    switch( v ) {
    case String(str): throw str;
    default:
    }
}
Instead of:
switch (e) {
    case Value(x):
        return x;
    case Add(x,y):
        return eval(x) + eval(y);
    case Throw(v);
        switch( v ) {
            case String(str): throw str;
            default:
        }
}

I find the latter example much more readable than the proposed one. Opening curly should always mean additional indentation level.

back2dos · 2019-06-12T09:41:56Z

@ncannasse I fully agree with all of your suggestions but one:

I would like the official indentation for case to be on the same level as switch. This will honor the roots of Haxe pattern matching which is OCaml match where patterns are not indented.

I've noticed that this is your personal preference and I while I don't share it, I could live with it. That said, I think for one it's not a pragmatic choice (more on that later) and your line of argument is poor. Haxe's syntax is rooted in ECMAScript and since we're discussing concrete syntax, not semantics, if there's any argument to be made about "being true to the roots", then we should follow what's common in ECMAScript and you're proposing quite the opposite.

Now, we can go 12 rounds on "which is better" (as Alex proposed 😄), but I think it's quite irrelevant. My perception is that the vast majority of Haxe code is written with indented cases (probably because shocking as it may be the author's background is far more often in AS3 than in OCaml). I know we don't consider majoritarian decision for language design and I understand why, but: this is not language design.

We're trying to establish a standard, backed by merely by an opt-in tool to support it. We want the maximum of people to use it (otherwise, why have a standard?), while not forcing them to (I guess the most we can do is pushing for IDE integration by default, so that it's elevated to an opt-out). Assuming the non-indented style is used no more than 20% of the time¹ I wonder what the wisdom is in picking it. We'll just fragment the community. Once we have people tweaking one formatting setting, they'll start messing with the others as well, because why not. We'll have accomplished nothing.

^{1. I'll gladly volunteer to get actual data from GitHub's code search, on the condition that a *clear* majority will conclude this decision.}

ncannasse · 2019-06-12T10:03:35Z

I find the latter example much more readable than the proposed one. Opening curly should always mean additional indentation level.

It does just that, since the case content is indented. With the current default, you have one brace that produces two levels of indentation : one for the case + one for the case content. That's one too much if you consider curly braces --> indentation.

@back2dos I do certainly and respect your point of view regarding existing code, as I am often in defense of the side of keeping in mind all existing code. And of course I think such feature should be customizable by people using formater. Now we're talking about what should be the default behavior. And I think that promoting what I think is the most readable and maps the best the language syntax is good enough to qualify as "design" as justify a change. Please note that our switch syntax differs a lot from EcmaScript already : no breaks, pattern matching, etc.

ncannasse · 2019-06-12T10:05:35Z

Still, a bit surprising since it's the opposite of your switch-case indentation preference, where you want to avoid indentation. ;)

OTOH I find that wanting a single level of indentation for actual code (vs "case patterns") is quite consistent ;)

ibilon · 2019-06-14T12:36:23Z

OTOH I find that wanting a single level of indentation for actual code (vs "case patterns") is quite consistent ;)

To me the cases only make sense inside a switch, hence the indentation.
The same way you indent function inside a class despite it not being "actual code".

…reformat

Aurel300 · 2019-07-01T13:55:38Z

Ok, I resolved the conflicts; we should merge this now. There are some issues with the formatting, but the output is still better than before (it's consistent at least) and waiting for all the formatter issues would just mean putting this off forever.

Once these issues are fixed, we can reformat again:

Simn · 2019-07-01T15:56:52Z

Great work, thanks!

* reformat root std classes * reformat std sys classes * reformat std haxe classes * reformat eval classes * reformat cpp classes * reformat cs classes * reformat hl classes * reformat java classes * reformat js classes * reformat jvm classes * reformat lua classes * reformat neko classes * reformat non-extern flash classes * reformat php classes * reformat python classes * remove cs WeakMap * merge reformat * fix loadLazy format in cpp.Lib * fix Lambda.flatten format * post-merge reformat * fix * fix 2 * fix 3 * fix 4 ... * fix 5 ..... * fix 7 .........

Aurel300 added 15 commits June 10, 2019 19:50

reformat root std classes

ac5b80b

reformat std sys classes

19d7f39

reformat std haxe classes

4f271af

reformat eval classes

0e1c8d5

reformat cpp classes

6065e27

reformat cs classes

987fbb9

reformat hl classes

4bbceb4

reformat java classes

5c0d7f3

reformat js classes

2374ce4

reformat jvm classes

784de8e

reformat lua classes

f39254c

reformat neko classes

17d30c6

reformat non-extern flash classes

40730b7

reformat php classes

cfbbabb

reformat python classes

3eeb2d0

remove cs WeakMap

89d1123

Aurel300 marked this pull request as ready for review June 11, 2019 16:02

Aurel300 added 2 commits June 11, 2019 17:05

Merge remote-tracking branch 'upstream/development' into feature/std-…

2ed56af

…reformat

merge reformat

66e0c0a

ncannasse reviewed Jun 12, 2019

View reviewed changes

std/Lambda.hx Outdated Show resolved Hide resolved

ncannasse reviewed Jun 12, 2019

View reviewed changes

std/StringTools.hx Show resolved Hide resolved

ncannasse reviewed Jun 12, 2019

View reviewed changes

std/cpp/Lib.hx Outdated Show resolved Hide resolved

skial mentioned this pull request Jun 12, 2019

Haxe Roundup 483 skial/haxe.io#625

Closed

1 task

Aurel300 added 2 commits June 12, 2019 10:52

fix loadLazy format in cpp.Lib

62bb2aa

fix Lambda.flatten format

d505931

Aurel300 added 2 commits July 1, 2019 14:32

Merge remote-tracking branch 'upstream/development' into feature/std-…

fbe5133

…reformat

post-merge reformat

1b77bc1

Aurel300 added 6 commits July 1, 2019 15:10

fix

f83c29d

fix 2

bf734b9

fix 3

1d2c93e

fix 4 ...

52795e8

fix 5 .....

e4a9426

fix 7 .........

3bc04e8

Simn merged commit 8616359 into HaxeFoundation:development Jul 1, 2019

Aurel300 deleted the feature/std-reformat branch July 1, 2019 15:57

yuxiaomao mentioned this pull request Oct 31, 2024

[std][hl] Fix array pos check, force UInt #11810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reformat std #8408

Reformat std #8408

Aurel300 commented Jun 11, 2019 •

edited

Loading

kLabz commented Jun 11, 2019

ncannasse Jun 12, 2019

ncannasse Jun 12, 2019

Gama11 Jun 12, 2019

ncannasse Jun 12, 2019

Gama11 Jun 12, 2019

ncannasse Jun 12, 2019

Gama11 Jun 12, 2019

ncannasse Jun 12, 2019

Gama11 Jun 12, 2019

Aurel300 Jun 12, 2019

Gama11 Jun 12, 2019

ncannasse commented Jun 12, 2019

Gama11 commented Jun 12, 2019

Gama11 commented Jun 12, 2019

RealyUniqueName commented Jun 12, 2019

back2dos commented Jun 12, 2019

ncannasse commented Jun 12, 2019

ncannasse commented Jun 12, 2019

ibilon commented Jun 14, 2019 •

edited

Loading

Aurel300 commented Jul 1, 2019

Simn commented Jul 1, 2019

Reformat std #8408

Reformat std #8408

Conversation

Aurel300 commented Jun 11, 2019 • edited Loading

kLabz commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncannasse commented Jun 12, 2019

Gama11 commented Jun 12, 2019

Gama11 commented Jun 12, 2019

RealyUniqueName commented Jun 12, 2019

back2dos commented Jun 12, 2019

ncannasse commented Jun 12, 2019

ncannasse commented Jun 12, 2019

ibilon commented Jun 14, 2019 • edited Loading

Aurel300 commented Jul 1, 2019

Simn commented Jul 1, 2019

Aurel300 commented Jun 11, 2019 •

edited

Loading

ibilon commented Jun 14, 2019 •

edited

Loading