Skip to content

Update sorted function to natural sort based on child.text value #154

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rrajpuro
Copy link

@rrajpuro rrajpuro commented Mar 10, 2025

Fixes issue #153

Adding a lambda function key to return child.text so we can sort strings naturally.

Reasons for choosing natural sort over lexicographical sort:

  • ACL sequence numbers
  • Interface numbers (Ex: Sort interface Vlan2 before interface Vlan100)

@rrajpuro rrajpuro changed the title Update sorted function to sort based on child.text value Update sorted function to natural sort based on child.text value Mar 10, 2025
@jtdub jtdub requested review from jtdub and aedwardstx March 22, 2025 21:04
@@ -4,6 +4,7 @@
from itertools import chain
from logging import getLogger
from typing import TYPE_CHECKING, Optional, TypeVar, Union
from natsort import natsorted
Copy link
Contributor

Choose a reason for hiding this comment

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

natsort is an external dependency that would need to be installed to use use this fix. I'm not certain that we want to add the external required dependencies.

@aedwardstx
Copy link
Collaborator

@rrajpuro thank you for your interest and suggestion.

Children can be weighted via OrderRules such that they float to the top or sink to the bottom relative to other children in the same level of the hierarchy. Otherwise, children retain their original order. As much as practical, we want to retain the order of config lines from the original config text. When different sorting logic is required, I’d recommend creating a post_load_callback, or similar approach, to handle these special cases.

Also, we want to keep external dependencies to a minimum.

@rrajpuro
Copy link
Author

rrajpuro commented Apr 1, 2025

Hello!
I agree with the point about keeping external dependencies to a minimum.
But then, what is the point of the sort method?
sorted(self.children) does not sort these Hconfig objects

should we implement the __ne__, __lt__, __gt__, __le__, __ge__ methods?

Also, I was working on fixing a unified_diff behaviour. The all_children_sorted method does not work as expected there. Hence, unified_diff needs this or a similar fix.

@rrajpuro
Copy link
Author

rrajpuro commented Apr 1, 2025

        for child in sorted(self.children):

I have two alternatives for removing natsort dependency and fixing this

  1. Use the usual string compare:
        for child in sorted(self.children, key=lambda child: child.text):
  1. Use the simplest form of natural sort:
convert = lambda text: int(text) if text.isdigit() else text
alphanum_key = lambda key: [convert(c) for c in re.split('([0-9]+)', key.text)]
...
...
        for child in sorted(self.children, key=alphanum_key):

Let me know if any of these is preferrable.

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