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

Remove references to ForeignGAP #620

Merged
merged 1 commit into from
Feb 12, 2021

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Feb 12, 2021

These are now gone in the latest master branch. Instead we have GAP_jll.MPtr and GAP.GapObj is an alias for that (this is slightly annoying, actually, it'd be great if we could "rename" it, but that's for another day to worry about.

Perhaps instead of GAP_jll.MPtr this PR should use its synonym GAP.GapObj. However, Julia prints that as GAP_jll.MPtr, so I think for the user GAP_jll.MPtr might be better?

julia> GapObj
GAP_jll.MPtr

@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #620 (154e3af) into master (f3177e1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #620   +/-   ##
=======================================
  Coverage   76.63%   76.63%           
=======================================
  Files          46       46           
  Lines        3921     3921           
=======================================
  Hits         3005     3005           
  Misses        916      916           
Impacted Files Coverage Δ
pkg/JuliaInterface/gap/convert.gd 100.00% <ø> (ø)

Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Converning GapObj vs. GAP_jll.MPtr:
We use GapObj in several places in the documentation of GAP.jl, and advertise GapObj as a constructor for conversions to GAP.
I think GapObj looks nicer, but users will anyhow have to know about GAP_jll.MPtr since this is what Julia prints.

@fingolfin
Copy link
Member Author

Perhaps then we should use GapObj, but add something somewhere which explains the relation? But I am in a meeting now. In any case, this PR makes things better, albeit not perfect...

Perhaps you can merge this PR and then (if you feel like it / have time) do such a change?

@ThomasBreuer ThomasBreuer merged commit 2ce604b into oscar-system:master Feb 12, 2021
@ThomasBreuer
Copy link
Member

@fingolfin the suggested change is in pull request #622.

@fingolfin fingolfin deleted the mh/ForeignGAP branch February 12, 2021 15:05
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