Omnibus PR containing multiple improvements #24
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
==========================================
+ Coverage 89.24% 89.44% +0.20%
==========================================
Files 1 1
Lines 186 218 +32
==========================================
+ Hits 166 195 +29
- Misses 20 23 +3
Continue to review full report at Codecov.
|
DilumAluthge
changed the title
DilumAluthge
changed the title
| @@ -132,6 +142,8 @@ isempty(::Number) = false | |||
| isempty(::Nothing) = true | |||
| isempty(x) = false | |||
| isempty(x, i) = isempty(Core.getfield(x, i)) | |||
| isempty(::Type{T}, x) where {T} = isempty(x) # generic fallback | |||
There was a problem hiding this comment.
Can you walk through why this is needed? What's your use-case?
There was a problem hiding this comment.
This allows users to customize the behavior of isempty for their own structs without modifying the behavior of isempty for other types or committing any type piracy.
For example, suppose that I want missing to be treated as empty when serializing my structs. I can just do isempty(::Type{MyType}, ::Missing) = true.
There was a problem hiding this comment.
A little more explanation is here: #21
| @@ -686,4 +703,57 @@ mappings will be applied, and the function will be passed the Julia field name. | |||
| return f_applied | |||
| end | |||
|
|
|||
| """ | |||
| makeobj(::Type{Any}, obj::Any) | |||
There was a problem hiding this comment.
I wonder if we just call this construct; we already have that defined in StructTypes, but that's really what we're doing here, and custom types are still able to overload it if they want.
There was a problem hiding this comment.
Hmmm, personally I'd find it confusing if this functionality was also named construct.
Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
I agree 100%. I almost never make PRs this big. The only reason I did so in this case is that when I tried merging the individual PRs locally, I realized that there were a lot of merge conflicts between the individual PRs. |
|
@quinnj This is ready for a second round of review. |
|
Ok, merge this for now; but I'd like to work on the |
This PR makes the following changes:
applyfield!would never be applied to the 32nd field of a mutable struct (Bug: when callingapplyfield!on a mutable struct, the 32nd field is skipped #22)makeobjandmakeobj!functionsisemptyfor their own structsProject.tomlfrom1.1.0to1.2.0Fixes the following issues:
Fixes quinnj/JSON3.jl#78
Fixes quinnj/JSON3.jl#90
Fixes #15
Fixes #22
Closes the following pull requests:
Closes #16
Closes #19
Closes #20
Closes #21