Add sort option to automodsumm #182

nstarman · 2024-01-16T04:45:09Z

mhvk

Good idea! Some comments inline.

mhvk · 2024-01-16T14:16:32Z

sphinx_automodapi/automodapi.py

@@ -67,6 +67,11 @@
        Propagates the ``noindex`` flag to autodoc. Use it to avoid duplicate
        objects warnings.

+    * ``:sort:``
+        Sort the objects in the module alphabetically. The default is to
+        sort by the order they appear in the module (as specified by `__all__`


Looking at the actual code below, I see that without __all__, our use of dir(module) automatically sorts things. So, maybe rewrite as

Sort the objects in the module alphabetically also if they are explicitly given in ``__all__`` (if ``__all__`` is not present, the objects are found using `dir`, which always gives a sorted list).

Actually, writing this suggests that perhaps the default should be to sort also with __all__ and have a nosort option. In that case,

Do not sort the objects alphabetically, but use the order given by ``__all__``. This option has no effect if a module does not have ``__all__``.

But probably what you have makes a bit more sense.

mhvk · 2024-01-16T14:17:34Z

sphinx_automodapi/utils.py

@@ -81,6 +81,9 @@ def find_mod_objs(modname, onlylocals=False):
    else:
        pkgitems = [(k, getattr(mod, k)) for k in dir(mod) if k[0] != '_']

+    if sort:


Given that dir is guaranteed to sort, this could be moved inside the if statement. (This is true even if __dir__ is defined and returns something unsorted.)

nstarman · 2024-01-16T16:58:42Z

Maybe we should change the option from sort to alphabetize?
OTOH. @mhvk had an idea about grouping imports my module or some other logical grouping. Should sort not be a boolean flag, but an enumeration? Or would that be a new flag, groupby? E.g. then :groupby: + :sort: would group the objects and sort within each group.

codecov · 2024-01-16T17:03:13Z

Codecov Report

Attention: Patch coverage is 93.75000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.28%. Comparing base (5ab68d0) to head (2b01690).
Report is 4 commits behind head on main.

Files	Patch %	Lines
sphinx_automodapi/automodapi.py	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #182      +/-   ##
==========================================
+ Coverage   90.24%   90.28%   +0.03%     
==========================================
  Files          28       28              
  Lines        1179     1204      +25     
==========================================
+ Hits         1064     1087      +23     
- Misses        115      117       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mhvk · 2024-01-16T17:12:01Z

sort in python is pretty unambiguous, so I would just stick with it.

We do need to add a test...

sphinx_automodapi/tests/test_automodsumm.py

pllim · 2024-02-20T21:08:39Z

@nstarman , are you still interested in pursuing this?

nstarman · 2024-02-21T14:06:01Z

Yes. Thanks for putting it back on my radar.

nstarman · 2024-02-22T16:56:51Z

Hm. Not sure why the test is failing so early. I copied it from a test above!

pllim · 2024-02-22T17:01:14Z

I cannot tell why it is failing from the log. You might need to run this test locally and debug.

nstarman · 2024-04-18T21:50:09Z

I would welcome help on the test suite for this PR. The PR works when tested on Astropy, I just can't wrangle this library's test suite.

mhvk · 2024-04-18T23:44:22Z

@nstarman - I think I found the problem: you missed how :sort: is also dealt with in writing summary lines. And that showed a problem with the test too... See nstarman#1 (as I'm not a maintainer, I cannot push to your repository...)

nstarman · 2024-04-19T19:21:49Z

Thanks @mhvk! I just cherry-picked your commit from your branch https://github.com/mhvk/sphinx-automodapi/tree/sort-option

Signed-off-by: nstarman <nstarman@users.noreply.github.com>

nstarman · 2024-04-19T19:23:14Z

Rebased to fix conflict.

nstarman · 2024-04-19T19:36:22Z

Ping @eteq, I think this is now ready for review!

mhvk

I think this looks all good, but then I looked at it already. The one unfortunate thing about the last commit (mine...) is that it does a lot of unrelated blackening as well - this makes it much harder to review. Could you try adding that commit again without that happening? (I checked, my original commit did not do this). I will make review much easier for @eteq.

Signed-off-by: nstarman <nstarman@users.noreply.github.com> # Conflicts: # sphinx_automodapi/automodsumm.py

nstarman · 2024-04-20T16:36:44Z

Done! Had to google how VSCode can do saving w/out formatting (I have mine set up to run code quality tools on save).

pllim · 2024-04-22T13:25:32Z

sphinx_automodapi/tests/test_automodsumm.py

+"""
+
+
+def test_sort(tmpdir):


Since this is new test, no reason to use legacy tmpdir? Can you please switch to tmp_path? Thanks!

Unfortunately this will require a larger refactor, because run_sphinx_in_tmpdir requires a tmpdir object! (the .strpath)

Ah ok, I opened #188 . Thanks for the clarification!

pllim

Given this is opt-in, let's merge. Thanks!

Do you need an immediate release?

bsipocz · 2024-04-22T16:08:53Z

Do you need an immediate release?

I wonder whether we could clean out 1 or 2 more of the remaining PRs, so we don't end up with single PR releases :)
Most are adding functionality but don't include tests, so while it's a bit of work to wrap them up I don't expect it to be too complicated.

nstarman mentioned this pull request Jan 16, 2024

Alphabetize units docs astropy/astropy#15891

Merged

mhvk reviewed Jan 16, 2024

View reviewed changes

pllim requested a review from eteq January 16, 2024 21:26

nstarman commented Jan 17, 2024

View reviewed changes

sphinx_automodapi/tests/test_automodsumm.py Outdated Show resolved Hide resolved

bsipocz mentioned this pull request Feb 20, 2024

Fix nonascii object names #184

Merged

nstarman force-pushed the sort-option branch from d259d02 to e02d4ee Compare February 22, 2024 16:47

pllim added this to the 0.18.0 milestone Feb 22, 2024

nstarman added 4 commits April 19, 2024 15:22

Add sort option to automodsumm

5aa7911

Signed-off-by: nstarman <nstarman@users.noreply.github.com>

address review comments

d1e2194

Signed-off-by: nstarman <nstarman@users.noreply.github.com>

improve import

bd8e1db

Signed-off-by: nstarman <nstarman@users.noreply.github.com>

WIP test

0afd92a

Signed-off-by: nstarman <nstarman@users.noreply.github.com>

nstarman force-pushed the sort-option branch from 4bdd6d2 to dd7eb69 Compare April 19, 2024 19:22

nstarman marked this pull request as ready for review April 19, 2024 19:33

nstarman changed the title ~~WIP: Add sort option to automodsumm~~ Add sort option to automodsumm Apr 19, 2024

nstarman requested a review from mhvk April 19, 2024 19:35

mhvk approved these changes Apr 19, 2024

View reviewed changes

Ensure sort is done also in automodsumm file writing

2b01690

Signed-off-by: nstarman <nstarman@users.noreply.github.com> # Conflicts: # sphinx_automodapi/automodsumm.py

nstarman force-pushed the sort-option branch from dd7eb69 to 2b01690 Compare April 20, 2024 16:35

pllim reviewed Apr 22, 2024

View reviewed changes

nstarman requested a review from pllim April 22, 2024 14:34

pllim mentioned this pull request Apr 22, 2024

TST: Replace legacy pytest tmpdir with tmp_path #188

Open

pllim approved these changes Apr 22, 2024

View reviewed changes

pllim merged commit e7c3b5c into astropy:main Apr 22, 2024
18 checks passed

nstarman deleted the sort-option branch April 22, 2024 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sort option to automodsumm #182

Add sort option to automodsumm #182

nstarman commented Jan 16, 2024 •

edited

Loading

mhvk left a comment

mhvk Jan 16, 2024

nstarman Jan 16, 2024

mhvk Jan 16, 2024

nstarman Jan 16, 2024

nstarman commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading

mhvk commented Jan 16, 2024

pllim commented Feb 20, 2024

nstarman commented Feb 21, 2024

nstarman commented Feb 22, 2024

pllim commented Feb 22, 2024

nstarman commented Apr 18, 2024

mhvk commented Apr 18, 2024

nstarman commented Apr 19, 2024

nstarman commented Apr 19, 2024

nstarman commented Apr 19, 2024

mhvk left a comment

nstarman commented Apr 20, 2024 •

edited

Loading

pllim Apr 22, 2024

nstarman Apr 22, 2024

pllim Apr 22, 2024

pllim left a comment

bsipocz commented Apr 22, 2024

Add sort option to automodsumm #182

Add sort option to automodsumm #182

Conversation

nstarman commented Jan 16, 2024 • edited Loading

mhvk left a comment

Choose a reason for hiding this comment

mhvk Jan 16, 2024

Choose a reason for hiding this comment

nstarman Jan 16, 2024

Choose a reason for hiding this comment

mhvk Jan 16, 2024

Choose a reason for hiding this comment

nstarman Jan 16, 2024

Choose a reason for hiding this comment

nstarman commented Jan 16, 2024 • edited Loading

codecov bot commented Jan 16, 2024 • edited Loading

Codecov Report

mhvk commented Jan 16, 2024

pllim commented Feb 20, 2024

nstarman commented Feb 21, 2024

nstarman commented Feb 22, 2024

pllim commented Feb 22, 2024

nstarman commented Apr 18, 2024

mhvk commented Apr 18, 2024

nstarman commented Apr 19, 2024

nstarman commented Apr 19, 2024

nstarman commented Apr 19, 2024

mhvk left a comment

Choose a reason for hiding this comment

nstarman commented Apr 20, 2024 • edited Loading

pllim Apr 22, 2024

Choose a reason for hiding this comment

nstarman Apr 22, 2024

Choose a reason for hiding this comment

pllim Apr 22, 2024

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

bsipocz commented Apr 22, 2024

nstarman commented Jan 16, 2024 •

edited

Loading

nstarman commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading

nstarman commented Apr 20, 2024 •

edited

Loading