search

jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
177 stars 28 forks source link

UnboundLocalError: local variable 'param_id_suffix' referenced before assignment #227

Closed kartben closed 1 year ago

kartben commented 1 year ago

See https://github.com/jbms/sphinx-immaterial/blob/cf059973255ee2595005765b941c81b47bbcc380/sphinx_immaterial/apidoc/cpp/parameter_objects.py#L329

This should probably be declared/assigned outside of the for loop, as otherwise there might be situations where "UnboundLocalError: local variable 'param_id_suffix' referenced before assignment" error gets thrown here https://github.com/jbms/sphinx-immaterial/blob/cf059973255ee2595005765b941c81b47bbcc380/sphinx_immaterial/apidoc/cpp/parameter_objects.py#L374

2bndy5 commented 1 year ago

there might be situations where "UnboundLocalError: local variable 'param_id_suffix'

I don't see how because python (at least CPython) is surprisingly flexible about variables scoped to a function (sometimes annoyingly so). While param_id_suffix is set repeatedly in the root branch of the for loop, the param_id_suffix variable will still exist in the after the for loop exits.

All that just to say that the error won't occur (and why it hasn't in our doc builds). I do agree with you though. There's nothing specific about the for loop's scope that requires that assignment be executed on every iteration; it only needs to be defined before the for loop. Can you submit a PR as well?

kartben commented 1 year ago

there might be situations where "UnboundLocalError: local variable 'param_id_suffix'

I don't see how because python (at least CPython) is surprisingly flexible about variables scoped to a function (sometimes annoyingly so).

Well it's occurred to me :) Python 3.9.6 // Sphinx 5.3.0

All that just to say that the error won't occur (and why it hasn't in our doc builds). I do agree with you though. There's nothing specific about the for loop's scope that requires that assignment be executed on every iteration; it only needs to be defined before the for loop. Can you submit a PR as well?

It might be a side effect of maybe the for loop not even being executed once in my case? I have to admit I just went for the quick fix of moving the definition up. And I will submit a PR, yes. Thanks for the quick reply!

2bndy5 commented 1 year ago

It might be a side effect of maybe the for loop not even being executed once in my case?

Yeah, C++ parameter handling needs some work still. #112 has a proposed solution to make the parameters' cross-linking in Python and C-family domains more configurable.

2bndy5 commented 1 year ago

Thanks again for taking the time to notify and fix this us!

There a couple more changes in the works before this fix gets published in a release. It shouldn't be too long of a wait though.

kartben commented 1 year ago

No worries at all! I'm not personally depending on it being released so feel free to take your time. FWIW at this point I'm mainly interested in trying to cherry pick your work around an improved search to use it in our rtd-theme based documentation, so you might still hear more from me though:)

Thanks!

kartben commented 1 year ago

Hi @2bndy5! It looks like I might now actually be interested in seeing this making it into a proper release, as the image I'm using to build my doc does not have Node.js, hence I can't really pull the HEAD version of sphinx-immaterial from GitHub as it won't build. Thanks!

2bndy5 commented 1 year ago

Can you use the nightly from test-pypi? Your last PR that just got merged is available under 0.11.3.post1.dev14

kartben commented 1 year ago

I sure can! I didn't even know this existed :) Thanks, that did the trick.