Skip to content
This repository was archived by the owner on Jan 19, 2022. It is now read-only.

Conversation

@eridal
Copy link
Contributor

@eridal eridal commented Mar 19, 2020

This will make npm pack output the tarball contents
in an alphabetically and natural sort order.

The current implementation uses localeCompare with
the following options:

  1. sensitivity: base, to correctly handle non-ascii letters
    using their base ascii, so that á,â, ä, ã, å are
    all handled like a

  2. numbers: true, to handle numeric filenames as natural
    ascending order so that, for instance, file 10.txt appears after 9.txt


Convo from npm/rfcs#96

cc @ruyadorno @ isaacs

claudiahdz and others added 3 commits March 13, 2020 20:05
This will make `npm pack` output the tarball contents
in an alphabetically and natural sort order.

The current implementation uses `localeCompare` with
the following options:

1. **sensitivity**: `base`, to correctly handle non-ascii letters
  using their base ascii, so that `á`,`â`, `ä`, `ã`, `å` are
  all handled like `a`

1. **numbers**: `true`, to handle numeric filenames as natural
  ascending order so that, for instance, file `10.txt` appears after `9.txt`

---

Convo from npm/rfcs#96

cc @ruyadorno @ isaacs
@eridal
Copy link
Contributor Author

eridal commented Mar 19, 2020

Hmmm those tests are failing with EXDEV: cross-device link not permitted, which means that fs.rename(src, dst) failed due to src and dst being on different devices, like C:\ and D:\

The right way to fix that seems to me way out of the scope of this PR

Any pointers?

test/index.js Outdated
'numbers/11',
'numbers/20',
'package.json',
'README.md',

Choose a reason for hiding this comment

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

hi @eridal thanks for the contribution! this looks great ❤️

now one very small change I'd like to ask - I know the GitHub web interface order files in such a way that capitalized letters come first (I think it comes from some sort of unix standard but I'm really not sure about it, don't quote me on that heheheh) anyways, the point is that's a standard that people got used to and it would be nice if we could replicate the same thing here 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruyadorno thanks for your feedback

Yes, at first that was the order I had set in place. Then I thought that given different OSes have different standards (ie: case vs icase, folders first, ...) the more inclusive approach would be to sort naturally.

But now that you mentioned, using the same listing order than Github really makes sense to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I did not like much the way it ended up being, but if I'm reading correctly the standard for localeCompare we cannot sort this as a single operation.

What I'm seeing is that using caseFirst with values upper or lower will yield orders AaBbCcDd.. and aAbBcCdD... respectively.

Given that we want ABCD..XYZabc...xyz then the only option (that I see) is to solve this in a two-steps process.

Please let me know your thoughts about this

https://www.ecma-international.org/ecma-402/1.0/#CompareStrings

Comment on lines +105 to +106
// eslint-disable-next-line
return 'A' <= ch && ch <= 'Z'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is quite sad how standard makes writing such common idioms in a broken way, compare this:

-    return ch >= 'A' && ch <= 'Z'
+    return 'A' <= ch && ch <= 'Z'

@claudiahdz claudiahdz force-pushed the latest branch 23 times, most recently from ddb0dba to c42528b Compare March 24, 2020 23:25
@claudiahdz claudiahdz force-pushed the latest branch 4 times, most recently from 68f3130 to 029ff99 Compare March 26, 2020 19:46
@claudiahdz
Copy link
Contributor

Hi @eridal! You caught us there in an unstable state when you created this PR. I manually included your changes and pushed them to latest. Thank you for your contribution!

@claudiahdz claudiahdz closed this Mar 26, 2020
@eridal eridal deleted the feature/sorted-tarball-contents branch March 26, 2020 21:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants