Sort imports in Phobos #5478

wilzbach · 2017-06-12T06:29:22Z

This is a preparation of the upcoming Dscanner enforcement for sorted imports in Phobos.
Of course, this wasn't done by hand.

There are still a couple of false negative to filter out at the Dscanner plugin (e.g. dlang-community/D-Scanner#444), but this import sorting script was laying on my disk now for a couple of months and we need to sort the imports anyways before we can move ahead with enforcing sortedness.

CC @andralex @CyberShadow

As this is quite a big chunk, I would appreciate if I don't need to rebase this that often ;-)

wilzbach · 2017-06-12T06:31:07Z

std/algorithm/comparison.d

 @safe pure unittest
 {
-    import std.utf : byChar, byWchar, byDchar;
+    import std.utf : byChar, byDchar, byWchar;


@andralex if we want to enforce sortedness for all imports, we have to look over a few minor exceptions where the order makes a bit of sense.

I'm not too worried about the few hand-optimizations that could be done. I'd say sorted is the simple and effective way.

wilzbach · 2017-06-12T06:37:01Z

std/datetime/systime.d

    @safe unittest
    {
-        import core.time : msecs, usecs, hnsecs, nsecs;
+        import core.time : hnsecs, msecs, nsecs, usecs;


That's the other example I found where order made sense.

jmdavis · 2017-06-12T07:07:21Z

Sure, it makes sense to be sorting imports alphabetically in general, but does it really make sense to effectively make it an error when they're not sorted that way? That seems like it's getting really picky about something that doesn't really matter much, especially if most imports are local, and we have fewer, large blocks of import statements. And of course, forcing sorting causes problems in the cases where another order arguably makes more sense.

andralex · 2017-06-12T13:33:18Z

It would be odd to have the compiler enforce it, but in phobos it's reasonable. The problem with other orders is that everybody may have their own notion of the best order.

andralex · 2017-06-12T13:56:21Z

Thanks. Should I override jenkins?

andralex · 2017-06-12T13:56:49Z

Apparently I can... we really should get jenkins going.

CyberShadow · 2017-06-12T14:46:09Z

I'm really not sure about this one (for the reasons @jmdavis mentioned).

CyberShadow · 2017-06-12T14:52:03Z

I think this should be reverted.

The problem with other orders is that everybody may have their own notion of the best order.

This is a slippery slope towards having one single "normalized" form of the code, and deviating from it is punishable by a 10-minute edit/amend/push/wait-for-CI cycle, and if we continue moving in this direction, I can see it happening a lot. Having code follow a consistent style is good, but this is going beyond style - many of the changes make the code uglier simply for the sake of normalization. If we are to enforce that imports are sorted, then why not also enforce that local variables are also sorted? Why should it be allowed to write int minValue, maxValue; if int maxValue, minValue; is a more "normal" form?

wilzbach · 2017-06-12T15:06:03Z

This is a slippery slope towards having one single "normalized" form of the code, and deviating from it is punishable by a 10-minute edit/amend/push/wait-for-CI cycle,

FWIW You can run dscanner locally - it's one of the most popular D projects & some IDEs/editors even have native support.

Having code follow a consistent style is good, but this is going beyond style - many of the changes make the code uglier simply for the sake of normalization.

I only found two examples where order actually made sense.
The entire point of the normalization is that we don't need to spend time anymore on nitpicking the order of imports.

CyberShadow · 2017-06-12T15:16:26Z

You can run dscanner locally

This is going to be completely irrelevant to most submitters unless they are told how to do it during the submission process. Links hidden among walls of text don't count. Even then, it lessens the frustration only to some degree.

wilzbach · 2017-06-12T15:19:35Z

This is going to be completely irrelevant to most submitters unless they are told how to do it during the submission process. Links hidden among walls of text don't count. Even then, it lessens the frustration only to some degree.

This is getting OT a bit, but FWIW we do plan to show a hello word message to new contributors. Also will this message contain detailed instructions based on the errors, see e.g. dlang/dlang-bot#57

andralex · 2017-06-12T15:22:18Z

I think this should be reverted.

This causes no harm regardless of how we move forward, so please leave it in. Thanks.

This is a slippery slope towards having one single "normalized" form of the code, and deviating from it is punishable by a 10-minute edit/amend/push/wait-for-CI cycle, and if we continue moving in this direction, I can see it happening a lot.

Yah, if we go for the tooling approach we should make it trivial for people to make the check locally - perhaps even automatically as part of the make process.

A variety of successful projects have very strict rules for formatting. I know from a GNU tools maintainer that their "good diff" tool (I forgot the name) was a lifesaver.

wilzbach · 2017-06-12T15:31:45Z

Btw @CyberShadow we have had sortedness of imports as part of the DStyle for Phobos since some time now.

Yah, if we go for the tooling approach we should make it trivial for people to make the check locally - perhaps even automatically as part of the make process.

style_lint is already a Makefile target - adding it to unittest (or a more generic test) would be rather easy ...

A variety of successful projects have very strict rules for formatting. I know from a GNU tools maintainer that their "good diff" tool (I forgot the name) was a lifesaver.

... and we don't swear at people using the wrong comment style:
http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html

CyberShadow

I only found two examples where order actually made sense.

Please count again, I think you're off by one or two ... orders of magnitude.

CyberShadow · 2017-06-12T15:27:44Z

std/algorithm/comparison.d

 @safe pure unittest
 {
-    import std.utf : byChar, byWchar, byDchar;
+    import std.utf : byChar, byDchar, byWchar;


CyberShadow · 2017-06-12T15:27:59Z

std/complex.d

        if (op == "^^" && is(C R == Complex!R))
    {
-        import std.math : exp, log, cos, sin;
+        import std.math : cos, exp, log, sin;


CyberShadow · 2017-06-12T15:28:03Z

std/complex.d

        if (op == "^^" && isNumeric!R)
    {
-        import std.math : cos, exp, log, sin, PI;
+        import std.math : cos, exp, log, PI, sin;


CyberShadow · 2017-06-12T15:28:09Z

std/complex.d

    @safe pure nothrow @nogc
 {
-    import std.math : sin, cos;
+    import std.math : cos, sin;


CyberShadow · 2017-06-12T15:28:21Z

std/container/array.d

 if (!is(Unqual!T == bool))
 {
-    import core.stdc.stdlib : malloc, realloc, free;
+    import core.stdc.stdlib : free, malloc, realloc;


CyberShadow · 2017-06-12T15:41:39Z

std/process.d

 private void setCLOEXEC(int fd, bool on) nothrow @nogc
 {
-    import core.sys.posix.fcntl : fcntl, F_GETFD, FD_CLOEXEC, F_SETFD;
+    import core.sys.posix.fcntl : fcntl, FD_CLOEXEC, F_GETFD, F_SETFD;


CyberShadow · 2017-06-12T15:43:27Z

std/string.d

    import std.conv : to;
    import std.traits : EnumMembers;
-    import std.utf : byCodeUnit, byChar, byWchar;
+    import std.utf : byChar, byCodeUnit, byWchar;


CyberShadow · 2017-06-12T15:44:26Z

std/windows/registry.d

 }

-version(unittest) import std.string : startsWith, endsWith;
+version(unittest) import std.string : endsWith, startsWith;


CyberShadow · 2017-06-12T15:44:43Z

std/string.d

 public import std.algorithm.comparison : cmp;
-public import std.algorithm.searching : startsWith, endsWith, count;
-public import std.array : join, replace, replaceInPlace, split, empty;
+public import std.algorithm.searching : count, endsWith, startsWith;


CyberShadow · 2017-06-12T15:44:51Z

std/windows/syserror.d

+    import std.algorithm.searching : endsWith, startsWith;
    import std.exception;
    import std.string;
-    import std.algorithm.searching : startsWith, endsWith;


CyberShadow · 2017-06-12T15:48:52Z

This causes no harm regardless of how we move forward,

Not true, this is a regression in code readability. Many, many small regressions, but a step backwards nevertheless.

A variety of successful projects have very strict rules for formatting.

Formatting is one thing; normalization at the cost of readability is another. Which is it?

we have had sortedness of imports as part of the DStyle for Phobos since some time now.

The style guide is ambiguous on whether the rule applies to selective imports as well.

jmdavis · 2017-06-12T16:08:22Z

This is a slippery slope towards having one single "normalized" form of the code, and deviating from it is punishable by a 10-minute edit/amend/push/wait-for-CI cycle, and if we continue moving in this direction, I can see it happening a lot.

It happens to me almost every time I create a pull request. From my perspective, the CI stuff has largely made life far more annoying for minimal benefit.

Yah, if we go for the tooling approach we should make it trivial for people to make the check locally - perhaps even automatically as part of the make process.

That would improve things in some ways and make them worse in others. I didn't even know that the 'style' target existed until Seb pointed out it to me at dconf, but even knowing about it, I can't use it on my Ubuntu system, because it doesn't work with PIC=1. I've have enough trouble building the docs historically, because of all of the extra stray tools that the dlang.org makefiles want, forcing me to edit the makefile just to be able to build it without all of that extra junk so that I can see what the docs look like. Making phobos depend on other tools is just going to make things worse. Now, if all of the parts required are just D things that get built as part of the build, then fine, but I don't want to have to constantly fight the druntime and Phobos builds, because they're overly complicated in terms of what they require.

It seems to me that all of this extra stuff is making it harder to contribute, not easier.

andralex · 2017-06-12T16:45:47Z

That would improve things in some ways and make them worse in others. I didn't even know that the 'style' target existed until Seb pointed out it to me at dconf, but even knowing about it, I can't use it on my Ubuntu system, because it doesn't work with PIC=1.

Most definitely a smooth operation is essential for the success of tooling. Style checks should simply be part of the unittest build. cc @wilzbach can you please make that happen for all makefiles?

It seems to me that all of this extra stuff is making it harder to contribute, not easier.

There's a lot of value in having automated tools instead of reviewers point out simple things like brace placement, punctuations, indentation etc. It's much more effective for both contributors and reviewers.

jmdavis · 2017-06-12T17:11:56Z

There's a lot of value in having automated tools instead of reviewers point out simple things like brace placement, punctuations, indentation etc. It's much more effective for both contributors and reviewers.

Oh, it can definitely help, but simply not being picky about something makes it even easier. Some level of pickiness on style makes sense so that the code is reasonably consistent, but it really feels to me like it's gone too far. Even with proper tooling that points out problems locally before creating a PR, complaining about something like the fact that startsWith being imported before endsWith just creates extra work for the person creating the PR without really improving the code.

Regardless, as it stands, the end result is that many of us create PRs only to have the Circle CI scream at us over some nitpicky thing, and then we have to go through several cycles to make it happy.

andralex · 2017-06-12T17:35:32Z

@jmdavis most certainly the hodge-podge of styles present in Phobos right now does not make it easier for anyone involved.

@wilzbach how about we start with only sorting imports alphabetically, not symbols within imports. The original problem was that people missed the presence of an import and would add it again.

CyberShadow · 2017-06-13T17:55:52Z

@wilzbach how about we start with only sorting imports alphabetically, not symbols within imports.

#5481

wilzbach added 3 commits June 12, 2017 07:54

Sort imports

61717ec

Sort selective imports

998ad51

Sort multiple packages within one line

2070d86

wilzbach commented Jun 12, 2017

View reviewed changes

andralex approved these changes Jun 12, 2017

View reviewed changes

andralex added the Merge:auto-merge label Jun 12, 2017

dlang-bot merged commit 0553161 into dlang:master Jun 12, 2017

CyberShadow mentioned this pull request Jun 12, 2017

Revert "Sort imports in Phobos" #5479

Closed

wilzbach deleted the sort_imports branch June 12, 2017 15:32

CyberShadow reviewed Jun 12, 2017

View reviewed changes

CyberShadow mentioned this pull request Jun 13, 2017

Revert "Sort selective imports" #5481

Merged

Uh oh!

Sort imports in Phobos #5478

Sort imports in Phobos #5478

Uh oh!

Conversation

wilzbach commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdavis commented Jun 12, 2017

Uh oh!

andralex commented Jun 12, 2017

Uh oh!

andralex commented Jun 12, 2017

Uh oh!

andralex commented Jun 12, 2017

Uh oh!

CyberShadow commented Jun 12, 2017

Uh oh!

CyberShadow commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jun 12, 2017

Uh oh!

CyberShadow commented Jun 12, 2017

Uh oh!

wilzbach commented Jun 12, 2017

Uh oh!

andralex commented Jun 12, 2017

Uh oh!

wilzbach commented Jun 12, 2017

Uh oh!

CyberShadow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmdavis commented Jun 12, 2017

Uh oh!

andralex commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmdavis commented Jun 12, 2017

Uh oh!

andralex commented Jun 12, 2017

Uh oh!

CyberShadow commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

CyberShadow commented Jun 12, 2017 •

edited

Loading

CyberShadow commented Jun 12, 2017 •

edited

Loading

andralex commented Jun 12, 2017 •

edited

Loading