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

use the safer Object_createChild SBML methods. #225

Merged
merged 2 commits into from
Jul 25, 2022
Merged

Conversation

exaexa
Copy link
Collaborator

@exaexa exaexa commented Jul 18, 2022

This depends on sbmlteam/libsbml#252 to be merged,
because some of the functions were actually not exposed to the C API.

Creating the members from the parent objects seems safer because all XML
metadata and versioning info gets automatically propagated. Also the code is a
bit shorter.

This depends on sbmlteam/libsbml#252 to be merged,
because some of the functions were actually not exposed to the C API.

Creating the members from the parent objects seems safer because all XML
metadata and versioning info gets automatically propagated. Also the code is a
bit shorter.
@exaexa exaexa requested review from giordano and laurentheirendt and removed request for laurentheirendt July 18, 2022 12:39
@exaexa
Copy link
Collaborator Author

exaexa commented Jul 18, 2022

(PS. obviously this won't work until the linked patch is in SBML_jll. :D )

Copy link
Collaborator

@giordano giordano left a comment

Choose a reason for hiding this comment

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

I agree this pattern is a bit better, when it can be used 👍

@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #225 (d0284be) into master (8705bd1) will decrease coverage by 0.08%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
- Coverage   93.76%   93.68%   -0.09%     
==========================================
  Files          10       10              
  Lines         754      728      -26     
==========================================
- Hits          707      682      -25     
+ Misses         47       46       -1     
Impacted Files Coverage Δ
src/unitful.jl 100.00% <ø> (ø)
src/writesbml.jl 94.17% <96.55%> (-0.35%) ⬇️
src/interpret.jl 76.92% <0.00%> (+4.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8705bd1...d0284be. Read the comment docs.

@exaexa
Copy link
Collaborator Author

exaexa commented Jul 18, 2022

when it can be used

Patchdozer disagrees. This will be used everywhere. :D

@giordano
Copy link
Collaborator

This PR also removes all uses of WRITESBML_PKG_DEFAULT_*

# Level/Version/Package version for the package
const WRITESBML_PKG_DEFAULT_LEVEL = 3
const WRITESBML_PKG_DEFAULT_VERSION = 1
const WRITESBML_PKG_DEFAULT_PKGVERSION = 2

apart from where we get the URI of the fbc extension

SBML.jl/src/writesbml.jl

Lines 760 to 768 in 9577bc1

uri = ccall(
sbml(:SBMLExtension_getURI),
Cstring,
(VPtr, Cuint, Cuint, Cuint),
sbmlext,
WRITESBML_PKG_DEFAULT_LEVEL,
WRITESBML_PKG_DEFAULT_VERSION,
WRITESBML_PKG_DEFAULT_PKGVERSION,
)

which sounds a good improvement in terms of consistency!

@exaexa
Copy link
Collaborator Author

exaexa commented Jul 18, 2022

Yeah that was the point. I kindof hope we'll eventually be able to spew out e.g. SBML L2V4 by just specifying the correct version and not filling in anything L3-specific into the sbml structure.

@exaexa
Copy link
Collaborator Author

exaexa commented Jul 25, 2022

now depends on JuliaPackaging/Yggdrasil#5224

@exaexa exaexa merged commit 384dd91 into master Jul 25, 2022
@exaexa exaexa deleted the mk-safer-create branch July 25, 2022 11:00
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