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

Introduce global gap variable GAP_jl #1126

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Jan 24, 2025

Initial step towards #1053.

I would only call it resolved, once the special case for Julia.GAP is indeed removed.

@lgoettgens lgoettgens requested a review from fingolfin January 24, 2025 11:19
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 80.76923% with 5 lines in your changes missing coverage. Please review.

Project coverage is 74.52%. Comparing base (847b4e4) to head (8dbd623).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/JuliaInterface/gap/convert.gi 63.63% 4 Missing ⚠️
pkg/JuliaInterface/gap/JuliaInterface.gi 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1126   +/-   ##
=======================================
  Coverage   74.52%   74.52%           
=======================================
  Files          55       55           
  Lines        4714     4714           
=======================================
  Hits         3513     3513           
  Misses       1201     1201           
Files with missing lines Coverage Δ
gap/exec.g 100.00% <100.00%> (ø)
pkg/JuliaInterface/gap/JuliaInterface.gd 100.00% <ø> (ø)
pkg/JuliaInterface/gap/calls.gi 96.66% <100.00%> (ø)
pkg/JuliaInterface/gap/convert.gd 100.00% <ø> (ø)
pkg/JuliaInterface/gap/juliahelp.g 90.38% <100.00%> (ø)
pkg/JuliaInterface/read.g 88.00% <100.00%> (ø)
pkg/JuliaInterface/gap/JuliaInterface.gi 91.46% <80.00%> (ø)
pkg/JuliaInterface/gap/convert.gi 68.11% <63.63%> (ø)

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thinking this through: the immediate name for this object that comes to mind is just GAP, on the top level. But that somehow "feels" wrong, or at least pretentious for us to claim that name. It feels as if a global variable with this name should either not exist, or "belong" to the GAP library.

So GAP_jl then seems rather natural. I can't say I love it (no offense), but as my language makes clear, everything I am saying here is about "feeling" and so extremely subjective anyway.

Of course it somewhat breaks with the usual GAP naming conventions that suggest CamelCase for globals, but GAPJl or GAPJL don't seem seem better, and GAPJulia is worse.

I am pretty sure I'll get used to GAP_jl if we use it shrug.

If we do this, then we should also consider renaming the global Oscar object in OscarInterface to Oscar_jl (or rather: have both names, at least for a while, but move everything to use Oscar_jl).

All that said, I'd really like to hear from @ThomasBreuer, perhaps he sees something I am not or has some other ideas and suggestions.

@ThomasBreuer
Copy link
Member

I would expect that the variable name GAP in a GAP session describes some important object of this GAP session, for example we could have called the GAPInfo record just GAP.
Inside a GAP session, using the name GAP for a Julia module would indeed be irritating.

Since we really need a variable for this module, I think GAP_jl is fine.

I found Julia.GAP very elegant: It expresses that it is something in the Julia world, and relative to that, GAP is exactly what one means.

I think there is no naming problem in the other direction: In Julia, GAP stands for the Julia module GAP, and GAP.Globals stands for the objects on the GAP side --the only problem is the ambiguity of GAP in sentences like this.

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.

see the comment which I just made

(I had thought that I had approved this pull request on Friday.)

@fingolfin fingolfin enabled auto-merge (squash) February 7, 2025 08:11
@fingolfin fingolfin merged commit d061b94 into oscar-system:master Feb 7, 2025
169 checks passed
@lgoettgens lgoettgens deleted the lg/global-var-GAP_jl branch February 10, 2025 13:22
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