-
Notifications
You must be signed in to change notification settings - Fork 7
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
JSON3 and StructTypes compat for InterTypes #106
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
+ Coverage 91.05% 91.62% +0.56%
==========================================
Files 22 22
Lines 2136 1910 -226
==========================================
- Hits 1945 1750 -195
+ Misses 191 160 -31 ☔ View full report in Codecov by Sentry. |
@jpfairbanks @fivegrant this is ready to review; let's get this merged. |
""" | ||
struct JSONTarget <: SerializationTarget end | ||
|
||
# TODO: Should this be ::JSONTarget instead of ::Type{JSONTarget} so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO I think we should probably keep it as is since it somewhat matches the other parts of source:
ACSets.jl/src/ACSetInterface.jl
Line 63 in f520029
function default_parts_type(::Type{PT}) where PT <: PartsType Lines 8 to 18 in f520029
abstract type Default end struct DefaultVal{x} <: Default end default(::Type{DefaultVal{x}}) where {x} = x struct DefaultEmpty{T} <: Default end default(::Type{DefaultEmpty{T}}) where {T} = T() ACSets.jl/src/ColumnImplementations.jl
Lines 20 to 23 in f520029
Base.convert(::Type{T}, x::T) where {T>:Union{Nothing,AttrVar}} = x Base.convert(::Type{T}, x::T) where {T>:Union{Missing,AttrVar}} = x Base.convert(::Type{T}, x::T) where {T>:AttrVar} = x Base.convert(::Type{T}, x) where {T>:AttrVar} = convert(Base.typesplit(T, AttrVar), x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A counterargument though is that the platform-specific options can be contained in the JSONTarget struct instead of extra keywords.
Addresses #83.
REGRESSION: The full range of 64 bit integers is no longer supported.