Skip to content

Conversation

@user202729
Copy link
Contributor

@user202729 user202729 commented Apr 8, 2025

As in the title.

There will be no visible change to the user because these methods all start with _, only to Sage developers.

The reason is… there's a lot of confusion in downstream implementations, so it looks like whoever initially wrote that code wasn't aware of the convention. See #39908 .

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

⌛ Dependencies

@github-actions
Copy link

github-actions bot commented Apr 8, 2025

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

Return a Gap object.
Unlike :meth:`_libgap_`, this method returns an instance of
:class:`sage.interfaces.gap.GapElement`, which wraps an object
Copy link
Contributor Author

@user202729 user202729 May 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure if this is the intention, or maybe this can return an instance of sage.libs.gap.libgap.GapElement if the argument G is libgap — with careful implementation, the method can be made to work in both cases.

Not sure what's the advantage of doing that however. If I recalled correctly some other parts of the code also did that.

@fchapoton fchapoton self-assigned this Jun 29, 2025
@fchapoton
Copy link
Contributor

looks good to me. I will wait for the CI

Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let's move on

@vbraun vbraun merged commit d5a1370 into sagemath:develop Sep 21, 2025
28 of 31 checks passed
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.

3 participants