#6781: Library access to ecl
--------------------------------------------------+-------------------------
Reporter: nbruin | Owner: nbruin
Type: enhancement | Status:
positive_review
Priority: major | Milestone: sage-4.4
Component: interfaces | Keywords: ecl, library
Author: Nils Bruin | Upstream: N/A
Reviewer: Burcin Erocal, Karl-Dieter Crisman | Merged:
Work_issues: |
--------------------------------------------------+-------------------------
Changes (by burcin):
* status: needs_review => positive_review
Comment:
Replying to [comment:28 nbruin]:
> That all looks great. However, I think the PY_NEW call should really be
{{{
cdef EclObject obj = EclObject.__new__(EclObject)
}}}
> According to
>
http://wiki.cython.org/FAQ#CanCythoncreateobjectsorapplyoperatorstolocallycreatedobjectsaspureCcode.3F
> it seems that PY_NEW was a hack that was only required prior to Cython
0.12
Thanks for pointing this out. I didn't know the new convention.
> A lot of the includes are not necessary either for the code to seemingly
work properly. Wouldn't it be better to leave them out?
The includes weren't necessary at all after getting rid of the `PY_NEW`
call.
I uploaded a new patch attachment:trac_6781-review.take2.patch addressing
these comments. The new patch replaces attachment:trac_6781-review.patch.
> I am probably not allowed to give the patch a positive review, but I can
confirm that the original reviewer requested those changes to be made and
they look fine to me (the author of the original enhancement).
You can change the ticket to positive review. I reviewed your patch and
gave a positive review, if you're ok with my additional changes and think
that they deserve a positive review, then the whole ticket is good to go.
> Thanks for trying to unstall the inclusion of this patch!
I want to get the rest of the interface (#7377) merged ASAP as well.
Unfortunately we had a setback with #8645. I hope the latter gets resolved
soon so we can proceed.
Based on Nils' comments above, I am changing this to a positive review.
The patches to be applied are:
* attachment:ecllib.patch
* attachment:trac_6781-review.take2.patch
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/6781#comment:29>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
