Skip to content

Conversation

@oxinabox
Copy link
Contributor

@oxinabox oxinabox commented Oct 8, 2024

Like #26 this seemed an interesting intellectual exercise.
This functionality looks scary and technically it is internals.
But I have been using it in ExprTools.jl in like every version of julia back til 1.0
Having to use an internal method for this is another manifestation of JuliaLang/julia#20555

I put this in a helper function so that after #26 we could add it to that list also

@exaexa
Copy link
Member

exaexa commented Oct 8, 2024

OK actually to keep people from getting confused, could we make the type_accessors a complete standalone "visible" method? It is a bit different from accessors (they don't "eat" an actual object).

EDIT: maybe even type_methods or so at that point, to make this even clearer.

Other than that this looks wunderbar, thanks again!

PS I see you rebased, perfect. :D

@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a2e4441) to head (8073ccb).

Additional details and impacted files
@@            Coverage Diff            @@
##            master       #27   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          267       280   +13     
=========================================
+ Hits           267       280   +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oxinabox
Copy link
Contributor Author

oxinabox commented Oct 8, 2024

I think it is pretty clear from how it is printed:

julia> AbstractFBCModels.required_accessors()
[1] reactions(a::AbstractFBCModels.AbstractFBCModel) @ AbstractFBCModels none:0
[2] n_reactions(a::AbstractFBCModels.AbstractFBCModel) @ AbstractFBCModels none:0
[3] metabolites(a::AbstractFBCModels.AbstractFBCModel) @ AbstractFBCModels none:0
...
[11] save(a::AbstractFBCModels.AbstractFBCModel, path::String) @ AbstractFBCModels none:0
[12] load(::Type{A}, path::String) where A<:AbstractFBCModel @ AbstractFBCModels ~/repos/AbstractFBCModels.jl/src/io.jl:7
[13] filename_extensions(::Type{A}) where A<:AbstractFBCModel @ AbstractFBCModels ~/repos/AbstractFBCModels.jl/src/io.jl:25

but up to you.
Its getting late here so I will leave it with you, if that ok?
Feel free to push to this branch before you merge.

@exaexa
Copy link
Member

exaexa commented Oct 8, 2024

actually yeah with the types it looks completely transparent. Thanks a lot!

@exaexa exaexa merged commit 67e5483 into COBREXA:master Oct 8, 2024
3 checks passed
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.

3 participants