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

Merged
merged 2 commits into from
Feb 21, 2025

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."

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 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 sagemath#39369

### 📝 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.
- [x] I have linked a relevant issue or discussion.
- [x] 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

<!-- 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#39499
Reported by: user202729
Reviewer(s):
@vbraun vbraun merged commit a01815f into sagemath:develop Feb 21, 2025
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants