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

Allow GAP.Obj(x,true) for recursive conversion #910

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

fingolfin
Copy link
Member

This makes it much more convenient to use from GAP: Julia.GAP.Obj(x,true)

TODO: document it somewhere, add a test

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #910 (124d5d1) into master (38af454) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #910      +/-   ##
==========================================
- Coverage   76.47%   76.43%   -0.04%     
==========================================
  Files          51       51              
  Lines        4166     4168       +2     
==========================================
  Hits         3186     3186              
- Misses        980      982       +2     
Impacted Files Coverage Δ
src/constructors.jl 96.20% <0.00%> (-2.50%) ⬇️

@ThomasBreuer
Copy link
Member

Concerning documentation, the obvious places are the docstrings for GAP.Obj and GapObj, which mention currently the keyword argument recursive.

However, this affects only the use from Julia. Since the remark mentions explicitly that the new syntax is intended for the use from GAP, the point is to mention it in the JuliaInterface manual. Currently we are advertising the GAP function (constructor) JuliaToGAP. If I understand the idea of this pull request right then it would be better to advertise Julia.GAP.Obj, for example because it covers also extensions from Oscar.jl, which JuliaToGAP does not know.

@fingolfin fingolfin closed this Jul 3, 2023
@fingolfin fingolfin reopened this Jul 3, 2023
@fingolfin fingolfin closed this Jul 18, 2023
@fingolfin fingolfin reopened this Jul 18, 2023
This makes it much more convenient to use from GAP: Julia.GAP.Obj(x,true)
@ThomasBreuer
Copy link
Member

@fingolfin Shall we merge this pull request, and then I create a new pull request for improving the documentation?

@fingolfin fingolfin merged commit 38e6f74 into oscar-system:master Jul 21, 2023
14 checks passed
@fingolfin fingolfin deleted the mh/Obj-no-kwarg branch July 21, 2023 14:36
@fingolfin
Copy link
Member Author

OK!

ThomasBreuer added a commit to ThomasBreuer/GAP.jl that referenced this pull request Jul 24, 2023
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.

2 participants