Skip to content

Conversation

@Techatrix
Copy link
Member

@Techatrix Techatrix commented Jan 3, 2023

This PR adds a code action for automatically sorting imports.

The current implementation has this sorting order:

const std = @import("std");
const builtin = @import("builtin");
const build_options = @import("build_options");

const known_folders = @import("known-folders");
const tres = @import("tres");

pub const offsets = @import("offsets.zig");
const Config = @import("Config.zig");
const debug = @import("debug.zig");
const Server = @import("Server.zig");

How exactly imports should be sorted needs a more thorough discussion.
Some of the questions that need to be answered include:

  • Should capitalization matter?
  • Should package and file imports be sorted by their declaration name or their string literal inside @import()?
  • Should their be line breaks between (std, builtin, builtin_options) packages and files?
  • Should it matter if a import declaration is pub or not?
  • How should @import("subdirectory/file.zig") sorted?
  • What about aliases like const Server = @This(); or const Ast = std.zig.Ast;?

There are also some issues in this implementation that need to be fixed:

  • preserve pub keyword and var or const keyword
  • preserve comments
  • move non imports to the bottom
  • detect this pattern: const name = @import("file.zig").Decl;

@leecannon
Copy link
Member

Just my two cents.

  • Should capitalization matter?
    Capitalization doesn't really factor in to "importance" so I don't think it should.
  • Should package and file imports be sorted by their declaration name or their string literal inside @import()?
    Declaration name is more straight forward and it is the name given by the user.
    But it would be nice that using the string literal would group related things together if directories are used, like this example:
const account = @import("account.zig");
const arch = @import("arch.zig");
const arm = @import("arch/arm.zig");
const riscv = @import("arch/riscv.zig");
const x86 = @import("arch/x86.zig");
const base = @import("base.zig");
const PhysAddr = @import("paging/PhysAddr.zig");
const VirtAddr = @import("paging/VirtAddr.zig");
  • Should their be line breaks between (std, builtin, builtin_options) packages and files?
    Personally I always seperate these things so that would be what I want but this is a perfect thing to have an option for.
  • Should it matter if a import declaration is pub or not?
    I think for a first cut not changing the order based on pub makes sense.
    What should it do if we did?
    Put the pub's first in the section they are in:
pub const build_options = @import("build_options");
const std = @import("std");
const builtin = @import("builtin");

pub const tres = @import("tres");
const known_folders = @import("known-folders");

pub const debug = @import("debug.zig");
const Config = @import("Config.zig");
const Server = @import("Server.zig");

Actually have differnet sections for pub vs non-pub:

pub const build_options = @import("build_options");

const std = @import("std");
const builtin = @import("builtin");

pub const tres = @import("tres");

const known_folders = @import("known-folders");

pub const debug = @import("debug.zig");

const Config = @import("Config.zig");
const Server = @import("Server.zig");

Or something else?

  • What about aliases like const Server = @This(); or const Ast = std.zig.Ast;?
    This is a good point towards sorting by decl name rather than import string. Maybe these should be in a fourth group (standard, packages, files, aliases)?

@matu3ba
Copy link

matu3ba commented Mar 14, 2023

Should capitalization matter?

Yes. Looks nicer.

Should package and file imports be sorted by their declaration name or their string literal inside

Prefix based sorting is simpler and reproducible with other tooling ([neo]vim has :sort for this).

Should their be line breaks between (std, builtin, builtin_options) packages and files?

KISS, if nobody has a feasible use case.

Should it matter if a import declaration is pub or not?
Having prefix-based sorting solves this aleady via simple rule.

Take a look at os.zig in my PR for how to make things nice and which stuff should be preserved (the posix stuff has empty line separations): https://github.com/ziglang/zig/pull/14726/files#diff-4284cdf770ea88be46f4aab7f82ed3c58821e4b3433b7c6152278a9911348dc5R21-R179

@sagehane
Copy link
Contributor

Just some bikeshedding: What does it mean for "capitalisation to matter"? I personally think it shouldn't matter in that adding more rules to sorting will make things annoying to deal with. Sorting things in order of ASCII/Unicode values make the most sense, imo. I personally prefer what sort with LC_ALL=C does.

@Techatrix Techatrix force-pushed the code-action-organize-imports branch from 80ace37 to 35a2a2d Compare April 1, 2023 22:36
@Techatrix
Copy link
Member Author

I'm gonna close this PR instead of keeping this old PR around forever. See #1637

@Techatrix Techatrix closed this Nov 28, 2023
@Sekky61 Sekky61 mentioned this pull request Oct 9, 2024
7 tasks
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.

4 participants