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.

Sign up for GitHub

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

Jump to bottom

Added generic IsJuliaWrapper #158

Merged
merged 1 commit into from
Nov 24, 2018
Merged

Added generic IsJuliaWrapper #158

merged 1 commit into from
Nov 24, 2018

Conversation

sebasguts
Copy link
Contributor

which can be used to wrap a Julia object and still be able to
call it on Julia functions

@codecov
Copy link

codecov bot commented Nov 14, 2018 •
edited
Loading

Codecov Report

Merging #158 into master will decrease coverage by 0.05%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
- Coverage   72.09%   72.03%   -0.06%     
==========================================
  Files          43       43              
  Lines        2297     2310      +13     
==========================================
+ Hits         1656     1664       +8     
- Misses        641      646       +5
Impacted Files Coverage Δ
JuliaExperimental/gap/utils.gi 100% <ø> (ø) ⬆️
JuliaInterface/src/JuliaInterface.c 95.37% <100%> (+0.05%) ⬆️
JuliaInterface/gap/JuliaInterface.gi 91.42% <100%> (ø) ⬆️
JuliaInterface/gap/JuliaInterface.gd 100% <100%> (ø) ⬆️
JuliaInterface/src/convert.c 86.48% <44.44%> (-13.52%) ⬇️

@sebasguts
Copy link
Contributor Author

@fingolfin @ThomasBreuer Should we merge this?

@ThomasBreuer
Copy link
Member

I would like to change JuliaExperimental to use this new feature,
thus it would be good to have it in the master branch.
Just the test failures are irritating.

@sebasguts
Copy link
Contributor Author

Give me some minutes to fix that, please.

@sebasguts
Copy link
Contributor Author

@ThomasBreuer On second thought, this is not super easy to fix atm, because we need to disable the automatic conversion for functions for this to work (i.e., we need to call julia_gap in the function handler).

I suggest we try to merge #160 today, and then this PR.

@ThomasBreuer
Copy link
Member

@sebasguts Fine with me.

@fingolfin
Copy link
Member

Clearly merging PR #160 "today" did no quite work out... And to fix JuliaExperimental in PR #160, we need this PR... so either we merge PR #160 ASAP, or else, the work in this PR is also added to PR #160, so that we can work on fixing JuliaExperimental there

fingolfin
fingolfin reviewed Nov 22, 2018
View reviewed changes
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.

Just some minor remarks. Overall, I like this PR (perhaps not surprising, given that I suggested it ;-)

JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/src/convert.c Outdated Show resolved Hide resolved
@sebasguts
Copy link
Contributor Author

I rebased the PR and adressed the comments.

fingolfin
fingolfin reviewed Nov 22, 2018
View reviewed changes
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/src/convert.c Outdated Show resolved Hide resolved
@sebasguts
Copy link
Contributor Author

@fingolfin I think I have adressed all your comments

fingolfin
fingolfin reviewed Nov 23, 2018
View reviewed changes
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.

Sorry, still some nitpicks. But overall it's looking pretty good now to me, thank!

JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
JuliaInterface/gap/JuliaInterface.gd Outdated Show resolved Hide resolved
@fingolfin fingolfin force-pushed the add_juliawrapper branch 2 times, most recently from 4a3400d to 37523a3 Compare November 23, 2018 12:48
@sebasguts @fingolfin
Added generic IsJuliaWrapper
55be249 
which can be used to wrap a Julia object and still be able to
call it on Julia functions
@fingolfin fingolfin changed the title WIP: Added generic IsTransparentJuliaWrapper Added generic IsJuliaWrapper Nov 23, 2018
fingolfin
fingolfin approved these changes Nov 23, 2018
View reviewed changes
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.

Tests now pass. @sebasguts perhaps you want to check that you are OK with the minor changes I made, then we can merge it.

@sebasguts
Copy link
Contributor Author

@fingolfin Yep, I am fine with the changes, thank you :)

@sebasguts sebasguts merged commit 5248496 into oscar-system:master Nov 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sebasguts @ThomasBreuer @fingolfin