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

feat: sort deno “node:/npm:” imports/exports first #524

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

denizdogan
Copy link

Makes it so that imports/exports that begin with node: or npm: are first in each set of declarations.

I would really like something like this to be way more fleshed out, with more fine-grained configuration options, but I'd like to see if there's any interest first. If not, I might make a separate dprint plugin with this stuff.

Some notes:

  • The Npm_ and Node_ names are meant to represent npm: and node:
  • The "inner text" of Node_ and Npm_ is just sorted alphabetically, i.e. not by path depth
  • I know these specifiers are Deno-specific, but it most likely will not hurt any non-Deno projects
  • Hope you don't mind I added a default VSCode test task

@CLAassistant
Copy link

CLAassistant commented May 13, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good, but instead of making this npm and node specific, perhaps we should make this "schemed"? So if a specifier starts with all letters then a colon it would be considered "schemed" and sort before "absolute", which could maybe be renamed to "bare". That way it's not Deno specific.

Relative { relative_count: usize, folder_text: &'a str },
Relative { count: usize, text: &'a str },
Npm_ { text: &'a str },
Node_ { text: &'a str },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have underscores here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were supposed to represent "npm:" and "node:" respectively, the underscore being the colon, but yeah, let me change those. :)

@denizdogan
Copy link
Author

Thanks, looks good, but instead of making this npm and node specific, perhaps we should make this "schemed"? So if a specifier starts with all letters then a colon it would be considered "schemed" and sort before "absolute", which could maybe be renamed to "bare". That way it's not Deno specific.

Yes, that makes sense. I'll revisit this soon. Thanks for the feedback!

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.

3 participants