Drop usage of internal API when conditionally including assets

[kartben]

Fixes #197 by reversing the logic of how JS/CSS assets are added to pages.

Rather than adding assets to the app and using undocumented API to remove them when not needed, do the exact opposite and only add them to the local html-page-context when building a page that uses the directive.

[kartben]

just realized I forgot to update pytests - should be green now

[ru-fu]

My Python skills don't suffice to review the changes in detail, but they look sensible.
I've tested the updated version and everything works as expected, and the warnings are gone.

Thanks a lot!

[kartben]

@foster999 codecov/codecov-action@v1 is probably due for an update since v4.5.0 is the current version. Maybe this will help handle rate limit and perform retries automagically? Do you want me to add a commit taking care of this to this PR?

[foster999]

Thanks for this @kartben 😄

@foster999 codecov/codecov-action@v1 is probably due for an update since v4.5.0 is the current version. Maybe this will help handle rate limit and perform retries automagically? Do you want me to add a commit taking care of this to this PR?

Yes please, might as well sort this at the same time! Failing that, I'd be happy to drop the codecov step in the Actions config for now.

Could you also drop the lines in the change for backwards compatibility with old sphinx (and if needed update the setup.py) please? I've tried to move towards only officially supporting the latest major version of dependencies, as older versions of sphinx-tabs can be used with older dependencies

This part of the code has been rewritten to and from this approach a few times, so I'd also like to understand why it was changed from the approach you've written here last time. I'll try and look back when I get chance, but it would be great if you could review older revisions/PRs to this part of the code to try to understand this also please

[simondeziel]

LGTM, thanks!

[kartben]

Yes please, might as well sort this at the same time! Failing that, I'd be happy to drop the codecov step in the Actions config for now.

I've pushed the change so let's see

Could you also drop the lines in the change for backwards compatibility with old sphinx (and if needed update the setup.py) please? I've tried to move towards only officially supporting the latest major version of dependencies, as older versions of sphinx-tabs can be used with older dependencies

Done

This part of the code has been rewritten to and from this approach a few times, so I'd also like to understand why it was changed from the approach you've written here last time. I'll try and look back when I get chance, but it would be great if you could review older revisions/PRs to this part of the code to try to understand this also please

I tried to look back in time a little bit but I couldn't really find a time where it ever was done like I am proposing in this patch. It's true that adding to all pages has apparently been done in different ways in the past, but I couldn't find occurrences of the "only add when used" approach.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.52%. Comparing base (e83dc14) to head (f8f2325).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   97.26%   98.52%   +1.26%     
==========================================
  Files           2        2              
  Lines         219      203      -16     
==========================================
- Hits          213      200      -13     
+ Misses          6        3       -3     

Flag	Coverage Δ
pytests	98.52% <100.00%> (+1.26%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[foster999]

Thanks for adding those bits and taking a look back. I think you're right - the old method just added them regardless of use, so the method being replaced here was likely a solution to that

As others have confirmed the issue is resolved, I'll get it merged and released once everything is passing

Edit: just looks like a formatting issue from pre-commit, then we're good to go

[kartben]

@foster999 I think we should be back on track.
Updated pre-commit.ci to fetch more recent versions of black, pylint, etc. since one of the errors was actually a hard crash due to a too old pylint version being pulled in and not being compatible with Python 3.12
Also dropped bad-continuation as per the warning that was showing up before the crash.
pre-commit run -a now passes locally

$ pre-commit  run -a
check json...............................................................Passed
check yaml...............................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
check-manifest...........................................................Passed
black....................................................................Passed
pylint...................................................................Passed

[simondeziel]

@foster999 if/when you get a chance to take another look that'd be great as the warning is polluting our build logs making it harder to spot any real issue. Thanks!

[foster999]

Sorry for the huge delay getting round to this! Now included in version 3.4.6

[kartben]

Sorry for the huge delay getting round to this! Now included in version 3.4.6

Awesome, thanks! Do you have a sense of when this will be available through pypi? Thanks!

[foster999]

Deployment failed for that one, but it's now up on PyPI as 3.4.7 ☺️

[simondeziel]

Sorry for the huge delay getting round to this! Now included in version 3.4.6

@foster999, no worries at all, thanks for your work as maintainer and also thanks again @kartben for the PR!