Issue/1732/custom sort

[bobwalker99]

No description provided.

[anirudnits]

@bobwalker99 Great work till now. Can you also add a unit test with the module numbers as referenced in the original issue?

[codecov]

Codecov Report

Merging #1752 (100347d) into main (4508b1a) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 100347d differs from pull request most recent head de7285d. Consider uploading reports for the commit de7285d to get more accurate results

@@            Coverage Diff            @@
##              main     #1752   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           37        37           
  Lines         2896      2909   +13     
  Branches       687       688    +1     
=========================================
+ Hits          2896      2909   +13     

[bobwalker99]

@bobwalker99 Great work till now. Can you also add a unit test with the module numbers as referenced in the original issue?

Thanks for the input @anirudnits - I've added a test as requested, test_custom_sort_python_unnatural does that look ok to you?

[anirudnits]

@bobwalker99 thanks, the test looks fine. Is it really needed to have importlib to import the sorting function? Like are we supporting custom sorting functions defined by the users? From what I can think of we need 2 sorting functions right? the already implemented one and the other "pythonic" one, considering alphabetical sorting and sorting by length are already present and the no-sort can be achieved by just using the only-sections option. In that case, we can have an option like sort-order will can one of 2 options default and pythonic with both being implemented in sorting.py and defending on the sort-order the corresponding function is invoked.

[bobwalker99]

@bobwalker99 thanks, the test looks fine. Is it really needed to have importlib to import the sorting function? Like are we supporting custom sorting functions defined by the users? From what I can think of we need 2 sorting functions right? the already implemented one and the other "pythonic" one, considering alphabetical sorting and sorting by length are already present and the no-sort can be achieved by just using the only-sections option. In that case, we can have an option like sort-order will can one of 2 options default and pythonic with both being implemented in sorting.py and defending on the sort-order the corresponding function is invoked.

Hi @anirudnits - I just did it like that in response to @timothycrosley’s comment:

This would make it easier as well to provide a way for users to write fully custom sorters, which has been an ask before as well.

Perhaps my approach is a step too far for now and more risky? I’m happy to be guided by you and/or @timothycrosley, I think it’s fairly straightforward to implement your suggestion above?
Appreciate your input as ever,
Regards,Bob

[anirudnits]

I think it’s fairly straightforward to implement your suggestion above?

Yeah I believe so, considering you've already solved for the more generic case, it should be fairly straightforward.

As for the usage of letting people define their own sorting functions, I think this would be useful if someone's using isort apis in their code and have niche use-cases. But for the CLI usage and CI integration, which I think comprises most of isort's use-cases, to write a separate function just for this purpose and pointing isort to use this seems extraneous to me, but maybe I'm narrow-minded on this and would be happy to go ahead with this approach if there are enough evidence to show otherwise.

~Thanks

[timothycrosley]

I think, if we want to support fully customizable sorting functions, beyond the API, instead of specifying the full callable path from the config, we should utilize a plugin based approach, such as is used for: https://github.com/PyCQA/isort/tree/main/example_isort_formatting_plugin. It probably makes sense to start out with just the API integration like @anirudnits is saying, and then if we do need more than that, implement plugin support for overriding the sorter

[bobwalker99]

Hi I made a mistake and had to recreate my branch but it auto-closed this PR, shall I just create a new one?

[anirudnits]

@bobwalker99 sure, just reference this PR in the new one so we could keep track of the conversation.