-
Notifications
You must be signed in to change notification settings - Fork 213
Are import shorthands worth the cost in readability/cognitive load? #1941
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
Comments
Note that If we shipped this feature with the |
I also worry about using I'm sure just about any other delimiter looks weird too though.
|
Perhaps something that contributes to confusion is that the new syntax (without quotes) is not distinguished from the old (with quotes) by a different token/delimiter. It is only different in that tokens (the quotes) are omitted. Adding a new (leading?) token may look strange, but catches the eye and makes very clear that a new style is in play. I think the quotes are characters that the brain doesn't really catch on. I'd have to look twice to see whether an import directive is using them or not. |
I actually find it's simple to understand, but it would really simplify things if we kept package imports and file imports seperate: package imports use the new proposal, and file imports use the old quoted syntax. // Assuming we are in lib/src/logic.dart of a package called my_package
import built_value; // 1. "package:built_value/built_value.dart"
import built_value:serializer; // 2. "package:built_value/serializer.dart"
import :data; // 3. "package:my_package/data.dart"
import "../data.dart"; // 4. Matches current behavior, imports lib/main.dart like #3
import "firestore.dart"; // 5. Matches current behavior, imports lib/src/firestore.dart
import "data/user.dart"; // 6. Matches current behavior, imports lib/src/data/user.dart This makes sense using the following rules:
Technically, I suppose that means EDIT: Regarding the difference between |
I do worry about overloading the I'm not worried about making code less readable - I think we're stripping away noise which should make it more readable. It's a high cognitive load to answer the questions I'm not worried about introducing too much choice for users - I think we can, and should, provide a single recommended pattern to use. I think the only "new" choices we are introducing are whether to use the old syntax or new (I think we recommend to use the new syntax) and whether to use The choice that's harder for us to give a universal recommendation on is package vs relative imports in the same package - and that choice is not changing with this proposal. I don't think the new syntax will have a higher cognitive load in the long run. It's a high cognitive load to think about exactly how to translate to the old syntax, but not necessarily a high cognitive load to understand what library the syntax represents. |
I personally don't think it is a high cognitive load, since the difference between package / relative imports already exists, the only change is that this gets rid of some of the noise. The only concern I see is the pub run syntax. However,
Why not deprecate the |
Personally, I don't see the value in the proposal. With IDE integration, I neither need to read nor need to edit imports. Even when there is a name conflict (which is rare), it's not the imports that I will look at, but the compiler error message which says "Class A is defined in both package_a and package_b". The only thing I really care about is exports. But exports primarily use relative file paths. |
Yes, I strongly believe it is. From the day we introduced pub to Dart, I have wanted a better import syntax. It's always been bad. Here's how you idiomatically import
Here's Dart current and proposed: import 'package:some_useful_thing/some_useful_thing.dart'; // Dart now
import some_useful_thing; // Dart shorthand Our current syntax is really verbose, even worse than JavaScript, which is saying something. We are the only language that requires a file extension in every import (!). We're the only one with essentially two keywords (
The inconsistency will be temporary and a fix is easily automatable. It would be less inconsistent if we'd designed a good syntax to begin with, but here we are. I don't think it's less readable. I think having to squint past the boilerplate
Don't forget I agree there are a lot of potentially different ways to refer to libraries but for any given library the guidance is pretty simple. It's:
Compare that to the current rules:
The only additional complexity is the shorthand for a package whose name is the same as the library, but that rule also avoids a very common source of redundancy. Users will have to be aware of both the old and new forms during the migration, which is a drag. I think it's worth the transition.
I think it's simpler to understand the semantics directly and not in terms of a desugaring to the old crappy syntax:
(There is also a special rule for handling dotted package names, but that's only meaningful for internal users.) I'm personally not too attached to (3), and would be OK with ditching the
There's numerous ways to create a list of integers too, but as long as there's a clearly better idiomatic one, it doesn't seem too onerous to me.
Only the last two of these are valid, and those are both valid today. |
How many other ways are there to import things in each of those languages? |
How much breakage would happen if the old import syntax was deprecated then later removed so that there is only one way to import? What invalid characters are people using that would cause issues? |
How about leaving quotes in as the default for relative imports? That way, invalid characters can still be supported From my earlier comment, you'd have this very concise list of options: // Assuming we are in lib/src/data/user.dart of a package called my_package
import built_value; // 1. "package:built_value/built_value.dart"
import built_value:serializer; // 2. "package:built_value/serializer.dart"
import :data; // 3. "package:my_package/data.dart"
import "profile.dart"; // 4. Matches current behavior, imports lib/src/data/profile.dart
import "../firestore.dart"; // 5. Matches current behavior, imports lib/src/firestore.dart |
Anyone should be familiar with those 3 types of imports:
Those 2 are questionable, to me at least, as it's not immediately obvious what the
The
or even get rid of it:
|
We can definitely ditch the I was once convinced by others that As for why not |
About the cognitive load, I think people will quickly learn the basic rules:
(Switched |
Technically, the Dart language allows any URI as import. Only Our tools (compilers included) support loading files from other URI schemas than those two, currently probably only The import shorthand will only apply to It also just applies to paths containing only ASCII letters, digits, dots and underscores, which again applies to almost all actual imports (I'm open to adding |
The compiler will know exactly waht to do. If you write Import shorthands, without quotes, cannot specify anything other than Unless you're writing stand-alone scripts, with no surrounding Pub package, that won't happen. Even those will likely use relative imports 99% of the time. |
I'm confused. Isn't the package name the first value in the path (here We also know it's a package because it does not start with one of:
What does "I'm inside" and "I do" mean in this case ? I parse this as "I write |
@cedvdb I agree that it makes no sense and you are going outside "the lib directory", but there is no "lib" visible in what looks like a path. That's why I prefer to separate the package name from the hierarchical path with a character other than It can certainly work to use You do |
@tatumizer You do know to push where it hurts, don't you 😝 Yes, that one annoys me too. It looks like it should be allowed. It's not, it is an error. It's currently not allowed grammatically. The grammar is very restrictive and says something like (from memory):
It's not a fully general "anything goes" grammar. You can't have embedded I do consider |
FWIW this feels less confusing to me as well. If I understand it correctly, with that model I really only need to think about two kinds of abbreviations:
Technically, there's still the old syntax for things that require escaping, but that's ok, you only use those for things that require escaping. Is that the intuition for dummies like me? |
@leafpetersen No, that sounds exactly right. And you can write (Or you can think of it as separate, but similar syntax for platform libraries only. In the long run, people will probably start to think of |
Now I remember the reason for preferring So, if Then Outside of We can allow both syntaxes, but that risks someone using That suggests just having one of them, which again points to Really, I'd like to be opinionated with this design (if you do something non-idiomatic, use a string URI instead), and it irks me to have two ways to do the same thing. And it also irks me to use WDYT? |
I think it would be very weird if |
Yes, to me |
FWIW, I 100% agree with the concerns that there's too much complexity here. I would go with only four forms (two of which exist today): import '../relative/path.dart'; // relative paths are strings and this works fine today
import 'package:foo/bar.dart'; // package imports can give precise paths and work fine today
import foo; // shorthand for importing "package:foo/foo.dart", most intuitive syntax, matches other languages
import foo.bar; // shorthand for importing "package:foo/bar.dart", matches other dot-delimited features in Dart The non-quoted forms would be either or ., nothing more complicated. This makes parsing it trivial and unambiguous to readers, and avoids introducing yet another kind of token for people to understand. |
|
I think We shouldn't let bazel affect how we design the language, IMHO. |
(All the popular languages that support a way to do this use periods, none of them use colons. I think that's an overwhelming argument in favour of periods.) |
I think a better mental model is that leading "/" or ":" (whichever syntax we choose) are package imports, not path ones. So it's basically:
|
@tatumizer I wouldn't expect any relative imports starting with The only character I've found in file/directory names other than [a-zA-Z0-9_.] (no |
Believe it! :) Or rather, ignore the grammar, look at the language. The grammar is an implementation detail. For something non-quoted, I want a measure of regularity. That covers all existing package names, directory names and file names in the Dart imports I checked. We could allow more, but at this point, I'm actually being opinionated and saying that you should stay inside that language if you want to use the quote-less imports. And yes, dots are special in that:
Maybe we should invent something new, from scratch, instead of this rather incremental approach, but I actually think it's a nice increment. |
@tatumizer
Both of those are wrong ways to do it. Files inside the If you also want to change how Dart recognizes and canonicalizes library import paths, and how it determines whether two different URIs represent the same library, we can do that too, but that's a bigger feature than just doing shorthands for the current URIs. This is considered a "small feature" because it's not changing anything about resolution, it's literally just shorthands for URI references.
You probably mean "library names" there. Currently, all Pub package names are simple identifiers, but I don't actually believe that will scale forever. I personally expect that we'll allow That said, if we did that, I also think Pub should allow aliases/renaming for packages, so you can depend on I can see how |
The thing is that if It's still a semi-magical link between package/path and dotted names, but all it means is that libraries without names, or with names that doesn't match their path, cannot be imported by their name. They can still be imported using their URI path. TL;DR: That idea, entirely separate from what we are doing here, would be to allow you to import packages by their name iff their name is their package name and path segments, without the trailing |
The problem here is that the compiler, seeing |
OK, here's another pitch: import dart/isolate; // -> 'dart:isolate';
import flutter_test; // -> 'package:flutter_test/flutter_test.dart';
import path; // -> 'package:path/path.dart';
import flutter/material; // -> 'package:flutter/material.dart';
import analyzer/dart/ast/visitor; // -> 'package:analyzer/dart/ast/visitor/visitor.dart';
import widget.tla.server; // -> 'package:widget.tla.server/server.dart';
import widget.tla.proto/client/component; // -> 'package:widget.tla.proto/client/component.dart';
import 'test_utils.dart';
import '../util/dart_type_utilities.dart';
import '../../../room/member/membership.dart';
import 'src/assets/source_stylesheet.dart';
import 'user_address.g.dart'; The rules are:
That's it. In other words, it's the original proposal but with no provision for unquoted relative paths, no rule for the current package, and It doesn't support unquoted relative imports because:
It uses a slash as the package name and library path separator because:
Aesthetically, I think it looks pretty nice. |
Here's what some files would look like: import flutter/src/rendering/sliver;
import flutter/src/widgets/basic;
import flutter/src/widgets/framework;
import flutter/src/widgets/layout_builder;
import flutter/src/widgets/scroll_view;
import flutter/src/widgets/sliver_layout_builder;
import flutter_test; import dart/async;
import dart/io as io;
import args/command_runner;
import file/memory;
import flutter_tools/src/base/common;
import flutter_tools/src/base/error_handling_io;
import flutter_tools/src/base/file_system;
import flutter_tools/src/base/io;
import flutter_tools/src/base/signals;
import flutter_tools/src/base/time;
import flutter_tools/src/build_info;
import flutter_tools/src/cache;
import flutter_tools/src/pub;
import flutter_tools/src/globals as globals;
import flutter_tools/src/pre_run_validator;
import flutter_tools/src/reporting/reporting;
import flutter_tools/src/runner/flutter_command;
import test/fake;
import '../../src/common.dart';
import '../../src/context.dart';
import '../../src/test_flutter_command_runner.dart';
import 'utils.dart'; https://github.com/flutter/flutter/blob/master/dev/bots/prepare_package.dart: import dart/async;
import dart/convert;
import dart/io hide Platform;
import dart/typed_data;
import args;
import crypto;
import crypto/src/digest_sink;
import http as http;
import path as path;
import platform show Platform, LocalPlatform;
import process; import dart/convert show JsonEncoder;
import flutter/material;
import flutter_driver/driver_extension;
import flutter_gallery/demo_lists;
import flutter_gallery/gallery/app show GalleryApp;
import flutter_gallery/gallery/demos;
import flutter_test;
import 'run_demos.dart'; That's not too bad. The slash isn't as satisfying as the dot for a separator IMHO, but it's clearly intuitive, and there's a path component to this, so it is sound from a conceptual point of view. It solves the verbosity problems cleanly. I could get used to the slash, I think. The syntax is simple and the mapping from this syntax to the old syntax is easy to explain. SGTM. |
Imo Having 2 different rule set for relative and package imports does not make imports simpler. I don't want to have to think whether I should add the extension or not. I'd prefer dropping the extension in all cases for relative imports to keep both streamlined. Also, and this is purely a preference, I'd prefer to use a dot for relative a la typescript A relative import is one that starts with ./ or ../. But quotes are fine too I guess, although quotes are more verbose.
|
That's already true today. Both forms are quoted, but you still have to know about the semi-magical "package:" URL scheme. |
To refer to the current package ( import dart/async;
import dart/io as io;
import args/command_runner;
import file/memory;
import test/fake;
import ~/src/base/common;
import ~/src/base/error_handling_io;
import ~/src/base/file_system;
import ~/src/base/io;
import ~/src/base/signals;
import ~/src/base/time;
import ~/src/build_info;
import ~/src/cache;
import ~/src/pub;
import ~/src/globals as globals;
import ~/src/pre_run_validator;
import ~/src/reporting/reporting;
import ~/src/runner/flutter_command;
import '../../src/common.dart';
import '../../src/context.dart';
import '../../src/test_flutter_command_runner.dart';
import 'utils.dart'; If this prefix doesn't bring syntax problem, this proposal makes the package's renaming a no-op, makes imports of current package shorter, allows grouping current package imports and has a relatively obvious meaning imho. |
import '/src/helper.dart'; works today (has worked since SDK version 2.13). There is no way to refer to the root of the pub package. I can live with that. As Bob makes a point of, his proposal only supports package URIs, and outside of About using I actually think that distinguishing imports of the current package from other imports is a good thing for readability. import "package:helper/helper.dart";
import "package:foo/foo.dart";
import "package:test/test.dart"; it doesn't stand out that |
I like @munificent's proposal, with the personal preference of using
In other words,
and
Some examples: import dart:isolate;
import flutter_test;
import path;
import flutter:material;
import analyzer:dart/ast/visitor;
import widget.tla.server;
import widget.tla.proto:client/component;
import 'test_utils.dart';
import '../util/dart_type_utilities.dart';
import '../../../room/member/membership.dart';
import 'src/assets/source_stylesheet.dart';
import 'user_address.g.dart'; import dart:async;
import dart:io as io;
import args:command_runner;
import file:memory;
import :src/base/common;
import :src/base/error_handling_io;
import :src/base/file_system;
import :src/base/io;
import :src/base/signals;
import :src/base/time;
import :src/build_info;
import :src/cache;
import :src/pub;
import :src/globals as globals;
import :src/pre_run_validator;
import :src/reporting/reporting;
import :src/runner/flutter_command;
import test:fake;
import '../../src/common.dart';
import '../../src/context.dart';
import '../../src/test_flutter_command_runner.dart';
import 'utils.dart'; import dart:async;
import dart:convert;
import dart:io hide Platform;
import dart:typed_data;
import args;
import crypto;
import crypto:src/digest_sink;
import http as http;
import path as path;
import platform show Platform, LocalPlatform;
import process; |
But using a |
Yeah I was wrong on that, not sure why I thought so. It does make the migration slightly more involved.
Packages are already denoted by a import "package:flutter/material.dart"; import flutter:material; // IMO, represents a library in a package import flutter/material; // IMO, represents a path that happens to be a package |
Does the distinction have to be made between a library and a file ? Aren't they all in essence the same ? What is the difference between a file with library at the top that exports other files, and the same file without library at the top ? |
First of all, the presence of
When I say library, I'm not referring to the When you import your own files in your own package, you do so using a relative import, like But when writing the UI, you want to import Flutter, specifically the Material components. In that case, you don't want to focus on a specific file from Flutter, because you don't care how it's organized. You just want to say "give me all the Material components from Flutter". The current way of importing feels like you need to know a bit of the internals of Flutter, because you're spelling out a path: Of course, this is all possible because Flutter has a file called |
So it's to communicate the intent. That begs the question why does this have to be reserver for the root of a package then ? If I have a nested Given that you wrote this:
The following does not make much sens to me:
but I don't see why it would not be valid given your description |
Correct. Let's make this concrete; I structure my projects like this:
Here's what my // models.dart
export "src/models/model1.dart";
export "src/models/model2.dart"; So, if you wanted to import // main.dart
import "package:my-project/models.dart";
// or
import :models;
That's because you're mixing up the ideas of relative imports and package imports. My comment is only about importing from other packages, since you don't know how those are laid out and using the top-level (non- |
I fully believe that the precise syntax is not very important. That's probably why the discussion is so vicious 😁 I believe that Dart authors will quickly get used to any of:
The driving force behind the discussion about these is not usability, it's aesthetics. That's incredibly subjective. The For the other two, the preference seems to be driven by whether one sees Perception and aesthetics based on that perception. If we introduce a shorthand, we'll have to pick one approach, one perception, and make that the official one. |
What if the models folder was nested in a core library / folder ? so Would it be
if Which has usability implications and that is not entirely subjective |
@cedvdb I think you're extrapolating way too much from my comment. Pub conventions are that importable code should be directly in your My proposal to use @lrhn, I think there's no reason to choose between |
For those following along, I took this comment and fleshed it out into a full proposal here. |
Should the |
There was a proposal to allow
to serve as a valid identifier.
and, more generally, every "unconvertible" case (even containing the allowed Unicode characters) could become "convertible" automatically. |
That's the purpose they serve here too, it's just that we don't have any public facing package organization tools that use it that way. The |
import 'package:flutter/material.dart';
import flutter/material ;
import 'package:analyzer/dart/ast/visitor/visitor.dart';
import analyzer/dart/ast/visitor/visitor ;
import 'package:widget.tla.proto/client/component.dart';
import widget.tla.proto/client/component ;
While I did find this compelling, there's a slight nuance. The keyword import flutter/material;
import "src/material.dart"; Now, there are the quotes and the import dart:convert;
import flutter:material;
import otherPackage:something/inside;
import "src/material.dart";
import "src/something/deep/inside.dart"; |
Personally I'm in the team that: I don't think many people actually care about imports. IMO for most people, they'd likely rather have everything imported by default, and not have to type imports. So to me, working on the syntax for typing imports is wasted effort. I personally only care about them in the case of a name conflict. But the IDE could highlight imports with symbol conflicts when that happens. |
Reading the import shorthand proposal, I am struck at the level of complexity for a user that this is introducing. Consider this text:
This is an extraordinary amount of new cognitive load for a user. There are now somewhere around 8 ways to write an import of
serializer.dart
, some of which are only valid in certain locations, and all of which use overlapping syntax to mean different things (":something
means thepackage
part was elided, butfoo:something
means add thepackage:
back in, change the:
to a/
and add a.dart
"). There's numerous ways to write the same thing:foo
andfoo:foo
andpackage:foo/foo.dart
and:./foo
and.foo
and"./foo.dart"
and"foo.dart"
all might mean the same thing (I think? I'm a little lost in the weeds.).Is this really worth the shorter syntax? I really worry that we're making code significantly less consistent and readable in the name of brevity.
cc @lrhn @munificent @eernstg @jakemac53 @natebosch @bwilkerson
The text was updated successfully, but these errors were encountered: