Add sort option to automodsumm by nstarman · Pull Request #182 · astropy/sphinx-automodapi

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

mhvk

Good idea! Some comments inline.

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

sphinx_automodapi/automodapi.py


+    * ``: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

    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

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

		"""


		def test_sort(tmpdir):

Uh oh!

Conversation

nstarman commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

nstarman Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

mhvk Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

nstarman Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

nstarman commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mhvk commented Jan 16, 2024

Uh oh!

Uh oh!

pllim commented Feb 20, 2024

Uh oh!

nstarman commented Feb 21, 2024

Uh oh!

nstarman commented Feb 22, 2024

Uh oh!

pllim commented Feb 22, 2024

Uh oh!

nstarman commented Apr 18, 2024

Uh oh!

mhvk commented Apr 18, 2024

Uh oh!

nstarman commented Apr 19, 2024

Uh oh!

nstarman commented Apr 19, 2024

Uh oh!

nstarman commented Apr 19, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

nstarman commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

nstarman Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

pllim Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Apr 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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