Declare the documented non-exported API public - #280
Closed
ChrisRackauckas-Claude wants to merge 1 commit into
Closed
Declare the documented non-exported API public#280ChrisRackauckas-Claude wants to merge 1 commit into
public#280ChrisRackauckas-Claude wants to merge 1 commit into
Conversation
`SBML.Model`, the `SBML.Math` AST types, and the documented data accessors are the package's user-facing interface, but none of them are exported -- `using SBML` would otherwise inject generic names like `Model`, `Species` and `Version` into the caller's namespace. That leaves downstream packages with no machine-checkable way to tell intended API from internals, and tools like ExplicitImports flag every `SBML.Model` as reaching into a private name. Declare those names `public` so `Base.ispublic` reflects what the manual already documents. `public` needs Julia 1.11 and this package supports 1.6, so the declaration is an `eval(Expr(:public, ...))` behind a version guard; a bare `public ...` would be a syntax error on older versions. Parser helpers (`get_optional_*`, `get_string`, `_readSBML`, ...), the `Maybe` combinators, `sbml`, and the math helpers in `math.jl` are left alone, since they are implementation details that happen to have docstrings. Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Member
|
Pure LLM contributions are not acceptable, please reopen once this is reviewed and sliced into single-topic changes. Also note that the notion of "public" API for the structures is somewhat smudgy here, as it depends very much on what the current SBML semantics are (oit of our control). |
Contributor
|
What is wrong with the PR? It's a very narrow single topic. Is there a specific thing you don't think should be marked as public? |
Contributor
|
Do you instead want all of those removed from the documentation? |
Contributor
|
Bump |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opened as a draft — please ignore until reviewed by @ChrisRackauckas.
Problem
SBML.Model, theSBML.MathAST types (MathApply,MathIdent, ...) and the documented data accessors (extensive_kinetic_math,initial_amounts,seemsdefined, ...) are the package's user-facing interface. They are documented in the manual, but none of them are exported, and deliberately so:using SBMLshould not inject generic names likeModel,SpeciesorVersioninto the caller's namespace.The side effect is that there is no machine-checkable distinction between intended API and internals. Downstream packages have to reach for
SBML.ModelandSBML.MathApply(...)with no signal that these are supported, and linting tools such as ExplicitImports.jl flag every one of those accesses as a private-name dependency. This came up concretely in SBMLToolkit.jl, which buildsSBML.MathApplynodes and readsMathApply.argswhile walking SBML math trees (SciML/SBMLToolkit.jl#223).Change
Declare those names
public, soBase.ispublic(SBML, name)agrees with what the manual already says.publicis a Julia 1.11 feature and this package's compat floor is 1.6, so the declaration goes througheval(Expr(:public, ...))behind a@static if VERSION >= v"1.11"guard — a barepublic ...is a syntax error on older versions. No new dependency; on Julia < 1.11 it is a no-op.Declared public (51 names, all of which already have docstrings rendered on the function reference page):
types.jl—Maybe,VPtr(both appear in documented signatures; user-written converters areVPtr -> Nothing)structs.jl— the full struct family,SBMLObjectthroughModel, including theMathAST and theGeneProductAssociation/Rulehierarchiesversion.jl—Versioninterpret.jl—interpret_math,default_function_mapping,default_constantsunitful.jl—unitfulutils.jl—extensive_kinetic_math,fbc_flux_objective,kinetic_flux_objective,get_compartment_size,initial_amounts,initial_concentrations,isfreein,seemsdefined,test_suite_urlDeliberately left alone, as implementation details that happen to carry docstrings: the
readsbml.jlparser helpers (_readSBML,get_model,get_association,get_optional_*,get_string, ...),mayfirst/maylift,check_errors/get_error_messages,sbml, and everything inmath.jl(already labelled "Internal math helpers" in the docs). Happy to move any of these across if you consider them supported.Julia has no field-level visibility, so making a struct type public is also the strongest available statement about its documented fields.
docs/src/functions.mdnow says this explicitly, along with the exported-or-publicrule and theBase.ispubliccheck.Tests
New
test/public.jl, run fromruntests.jl:ispublicand not exportedusing SBMLis alsoispublicThat last one is the useful regression guard: it fails if an export is added without the maintainers thinking about the public surface.
Verification
Run locally, output observed:
Pkg.test()on Julia 1.12.6 —719 Pass, 1 Broken, 720 Total(the broken test is pre-existing onmaster)Pkg.test()on Julia 1.10.11 (CI LTS) —556 Pass, 1 Broken, 557 Totaltest/public.jlalone — 265 passing assertions on 1.12, 102 on 1.10 (theispublicassertions are correctly skipped below 1.11)using SBMLloads clean on 1.10.11, confirming the version guard does not break the pre-1.11 pathJuliaFormatter.format(".")produces no diff on the changed filesOne unrelated formatting drift the current JuliaFormatter wants in
test/loadmodels.jl(doc -> begin→doc->begin) was reverted to keep this diff focused.Note on versioning
This adds public API, so by SemVer it warrants a minor bump (1.6 → 1.7). I left
Project.tomluntouched since releases are yours to cut — say the word and I will add the bump.