Skip to content
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

Improve sage_getfile by looking at __init__ #39499

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Feb 12, 2025

Otherwise it will fail with meson editable install on objects like x of type Expression, where the class does not have a docstring but __init__ method have.

The strategy is similar to sage_getsourcelines where __init__ method is looked in.

I choose to implement this instead of the more complex workaround (see the comment below).

This fixes the test that fails in meson editable mode:

2025-02-11T20:13:36.4801998Z **********************************************************************
2025-02-11T20:13:36.4803076Z File "src/sage/misc/sageinspect.py", line 1363, in sage.misc.sageinspect.sage_getfile_relative
2025-02-11T20:13:36.4804104Z Failed example:
2025-02-11T20:13:36.4804750Z     sage_getfile_relative(x)                                                  # needs sage.symbolic
2025-02-11T20:13:36.4808395Z Expected:
2025-02-11T20:13:36.4813350Z     'sage/symbolic/expression.pyx'
2025-02-11T20:13:36.4814244Z Got:
2025-02-11T20:13:36.4815714Z     '/home/runner/work/sage/sage/builddir/src/sage/symbolic/expression.pyx'
2025-02-11T20:13:44.3128543Z **********************************************************************

This may still fail in another case where neither class nor __init__ method has a docstring (in that case ? will fail to get the file in meson editable mode), but that's not in scope I guess. (I left a comment there to explain)

See also #39369

📝 Checklist

  • The title is concise and informative.
  • 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.
  • I have updated the documentation and checked the documentation preview. (change should be invisible to users not using meson editable so…)

⌛ Dependencies

Copy link

github-actions bot commented Feb 12, 2025

Documentation preview for this PR (built with commit 6aee498; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@saraedum saraedum requested a review from tobiasdiez February 13, 2025 09:50
@orlitzky
Copy link
Contributor

You could use if hasattr(obj, '__init__') ... rather than try/except/else? Otherwise LGTM.

@user202729
Copy link
Contributor Author

I copy the general structure from sage_getsourcelines . Presumably so that the attribute access only need to be done once (hasattr internally works by trying to access the attribute and catch any exception raised)

Advantage is it would be 4 lines shorter I suppose.

@orlitzky
Copy link
Contributor

Thanks. For me it's just much easier to read when the logic is "if thing is true, do stuff that depends on thing."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants