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

Support keyword arguments in CallJuliaFunctionWithCatch, and a few other improvements #1043

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

ThomasBreuer
Copy link
Member

(motivated mainly by gap-packages/PackageManager/pull/134 and the issues mentioned there)

  • support keyword arguments in CallJuliaFunctionWithCatch

  • do not use the deprecated and undocumented Core._apply

  • update the initializations in init_packagemanager: Replacing PKGMAN_DownloadURL is no longer needed, replacing PKGMAN_RemoveDir was in a wrong place.

  • start better documentation of Julia syntax features

(motivated mainly by gap-packages/PackageManager/pull/134
and the issues mentioned there)

- support keyword arguments in `CallJuliaFunctionWithCatch`

- do not use the deprecated and undocumented `Core._apply`

- update the initializations in `init_packagemanager`:
  Replacing `PKGMAN_DownloadURL` is no longer needed,
  replacing `PKGMAN_RemoveDir` was in a wrong place.

- start better documentation of Julia syntax features
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.44%. Comparing base (785a93a) to head (4a847f0).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
pkg/JuliaInterface/gap/calls.gi 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1043      +/-   ##
==========================================
+ Coverage   74.39%   74.44%   +0.05%     
==========================================
  Files          55       55              
  Lines        4527     4540      +13     
==========================================
+ Hits         3368     3380      +12     
- Misses       1159     1160       +1     
Files with missing lines Coverage Δ
pkg/JuliaInterface/gap/JuliaInterface.gd 100.00% <ø> (ø)
src/packages.jl 72.89% <100.00%> (ø)
src/utils.jl 56.41% <100.00%> (+7.92%) ⬆️
pkg/JuliaInterface/gap/calls.gi 96.66% <90.90%> (-3.34%) ⬇️

pkg/JuliaInterface/gap/calls.gi Outdated Show resolved Hide resolved
src/packages.jl Outdated
global DOWNLOAD_HELPER[] = Downloads.Downloader(; grace=0.1)

# overwrite PKGMAN_DownloadURL
Copy link
Member

Choose a reason for hiding this comment

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

Did you verify that PackageManager still uses Julia's download feature? I am asking because the weird error about "could not import Downloads" suggests to me that perhaps JuliaImportPackage("Downloads") returns false and so the PackageManager code is not being loaded...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this was also my understanding:
Whenever the warning occurs, the overwrite inside PackageManager does not happen, but up to now, we were anyhow overwriting the function afterwards, thus the code in PackageManager was actually never used.

With gap-packages/PackageManager/pull/134, the problem with the warning should disappear. It depends on the version of PackageManager used whether we can safely remove the code from GAP.jl.

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.

I am happy with this but I'd suggest we wait until after PR #1029 is in, just to make sure we don't delay that one further.

@fingolfin fingolfin changed the title a few improvements Allow passing keyword arguments via call_with_catch, and a few other improvements Sep 23, 2024
@fingolfin fingolfin changed the title Allow passing keyword arguments via call_with_catch, and a few other improvements Support keyword arguments in CallJuliaFunctionWithCatch, and a few other improvements Sep 23, 2024
@fingolfin fingolfin added the kind: enhancement New feature or request label Sep 23, 2024
@fingolfin fingolfin merged commit e5c5358 into oscar-system:master Sep 24, 2024
21 checks passed
@ThomasBreuer ThomasBreuer deleted the TB_catch_kwarg branch September 24, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants