-
-
Notifications
You must be signed in to change notification settings - Fork 652
Show signature for binding=False cython functions #39279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
sage: def f(x, y, z=1, t=2, *args, **keywords): | ||
....: pass | ||
sage: sage_signature(f) | ||
<Signature (x, y, z=1, t=2, *args, **keywords)> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
examples mostly copied from sage_getargspec, modified accordingly.
Documentation preview for this PR (built with commit 1232337; changes) is ready! 🎉 |
Would this be much work? Since this seems to be 'just' a cython-specific problem, and nothing intrinsically related to sage, I would prefer if this could be fixed upstream in IPython (in fact, I hope that most of the special Cython handling in |
Might be worth trying, let's see. |
(at the moment I don't really have time to port the change to IPython, nor to deal with the various versioning issue e.g. if ipython version ≥ X implements this, then sage need to be sure to not worsen the situation by monkey patching its own function into ipython if version is ≥ X. On the other hand it's comparatively easier to implement this directly into sage since the framework is already there. We can just get it in first then do the refactor later, which also has the added benefit of not having to wait until the next ipython release to have the feature available?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's get it in; fixing this upstream and converting to binding=true
in sage is still desirable in my opinion.
PDF docbuild fails
|
Ah, looks like it failed on CI too. I guess the CI is not reliable enough for me to notice it…
[trying to diagnose the issue] it's using batchmode which hides the issue… (src/doc/Makefile → doc-pdf) (also confusingly it's named [scroll up]
so I try
it's compiling.
→ I look at the
Looks like the same line. In
Why does the issue happen… I think building docs sort of mostly work in meson, would be nice (and save electricity) if that can be converted to meson too… easier said than done though. |
Should be fixed now, I believe. Lesson to be learnt when AI output is not checked properly I suppose… |
@tobiasdiez Can you review again, thanks. (actually previously the PDF build failure is also reflected by the formulas not rendering correctly on the documentation preview, both works now including the documentation preview) |
sagemathgh-39939: Add sphinx-copybutton to dependency of sage-docbuild Reason: the package doesn't run correctly without the dependency. It is listed as a dependency in the sage spkg too (`build/pkgs/sagemath_doc_html/dependencies`) First noticed while working on sagemath#39279 . ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39939 Reported by: user202729 Reviewer(s): Tobias Diez
sagemathgh-39939: Add sphinx-copybutton to dependency of sage-docbuild Reason: the package doesn't run correctly without the dependency. It is listed as a dependency in the sage spkg too (`build/pkgs/sagemath_doc_html/dependencies`) First noticed while working on sagemath#39279 . ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39939 Reported by: user202729 Reviewer(s): Tobias Diez
sagemathgh-39939: Add sphinx-copybutton to dependency of sage-docbuild Reason: the package doesn't run correctly without the dependency. It is listed as a dependency in the sage spkg too (`build/pkgs/sagemath_doc_html/dependencies`) First noticed while working on sagemath#39279 . ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39939 Reported by: user202729 Reviewer(s): Tobias Diez
Fixes #26254 , without binding=True. (That is, this shows the signature correctly in
function?
for certain functions compiled in Cython.)Before:
After:
Even if we ultimately decide to set binding=True, this would probably still be desirable, in case some other library chooses to use binding=False, embedsignature=True.
Note: there's an alternative of implementing this into IPython, see ipython/ipython#14639
📝 Checklist