Sort by crate lexicographical order

[bebert64]

Fixes #511.

First time contributing to an open-source project, so please let me know if I've done anything wrong or how I can improve. Also should I squash my commits before submitting the PR?
And running cargo fmt completely changes the way the imports are structured, so I ended up saving without formatting, but I'm not sure if that's what's intended

[ia0]

Thanks for the PR!

For coherence, I think we should use the same value for Eq and Ord (even though for well-formed keys it shouldn't matter for the current PR):

• either the cleaned key (String)
• or the dot-separated cleaned key (Vec<String>)

[bebert64]

Thanks for the PR!

For coherence, I think we should use the same value for Eq and Ord (even though for well-formed keys it shouldn't matter for the current PR):

* either the cleaned key (`String`)

* or the dot-separated cleaned key (`Vec<String>`)

The whole issue being that we need to compare the Split instead of the String itself, I believe the only option is then to use the Split for Eq as well.

[ia0]

The whole issue being that we need to compare the Split instead of the String itself, I believe the only option is then to use the Split for Eq as well.

Yes, we should use the same value for all comparisons: Eq and Ord. The choice between "foo.bar" and ["foo", "bar"] doesn't make a big difference to me. It's true the second is a bit cleaner, but the first is also simpler. I'm fine with both.

[bebert64]

The whole issue being that we need to compare the Split instead of the String itself, I believe the only option is then to use the Split for Eq as well.

Yes, we should use the same value for all comparisons: Eq and Ord. The choice between "foo.bar" and ["foo", "bar"] doesn't make a big difference to me. It's true the second is a bit cleaner, but the first is also simpler. I'm fine with both.

I'm not sure I understand. If we use "foo.bar" for Ord, don't we have exactly the same problem as before?

[ia0]

I'm not sure I understand. If we use "foo.bar" for Ord, don't we have exactly the same problem as before?

Sorry, what I meant was "foo.bar." like I initially suggested in #511 (comment).

[bebert64]

Sorry, what I meant was "foo.bar." like I initially suggested in #511 (comment).

I apologize, I still must have misunderstood something. From what I had gathered, the problem comes from the fact that the '-' comes before the '.', which leads to "foo-.bar" to come before "foo.bar", even though "foo" comes before "foo-" (and we wanted those two ordering to be consistent). If that is correct, the "foo.bar." will still come after "foo-.bar.", no? On the other hand, ["foo", "bar"] would correctly come before ["foo-", "bar"].

[ia0]

I thought the problem was that the order between foo = { bar = 0 } and foo- = { bar = 0 } was inconsistent with the order between foo.bar = 0 and foo-.bar = 0.

With my solution, if I understand correctly, the order would be the same because in both cases we would be comparing foo.bar. with foo-.bar. (or more precisely, foo. with foo-. on one side and foo.bar. and foo-.bar. on the other).

But if you not only want consistency, but also proper ordering (i.e. one of foo or foo- is supposed to be first), then indeed there's only room left for ["foo", "bar"] vs ["foo-", "bar"].

[bebert64]

Ok, got it now, thanks. Then I think I don't have new modifications to write. I'm not sure what's the correct process now: should I re-ask for a review or is that implied by this conversation?

[bebert64]

I apologize, I realize my previous message was far from clear 🥲 Thanks to your explanations, I now see how your solution provides consistency, but I would prefer the solution where that ordering is also consistent with the "classical" alphabetical order (foo before foo-bar). Since it's comparing iterator, I don't think there is a downside in performance, and believe it's probably closer to what most people would expect. Would that be okay with you?

[ia0]

Yes, both were fine with me. So let's modify the cleaned_key to be Vec<String>.

[bebert64]

No problem, thanks, I'll push the Vec modification in a min. For my curiosity (still learning :) ): wouldn't that require more allocations than just splitting in place every time? Do you prefer it because it's easier to read at the (arguably low) cost of a few additional allocation?

[ia0]

Yes, there's one more allocation, but this is not high performance code (as you might have seen by the previous code allocating strings on every comparison) so it's not a concern. However readability and correctness by construction is more important here. In particular the fact that the comparison key exists in only one place is the key factor. Even without the comparison key being cached I would have been fine (i.e. fn cmp_key(&self) -> Vec<String> being called at each comparison for both sides).

[ia0]

Looks good to me modulo the import formatting.

[bebert64]

Thanks for the explanations, very clear and makes a lot of sense!

[bebert64]

Looks good to me modulo the import formatting.

Thanks, I appreciate your patience and comments/explanations during my first contribution!

[ia0]

Thanks for the PR!

[bebert64]

Out of curiosity, how does the release process work ? I've seen that the commit has been released as part of release-taplo-0.13.0, and I was wondering how it translates in terms of the vscode extension. Thanks for your insights