Closed rofinn closed 6 years ago
Merging #51 into master will not change coverage. The diff coverage is
100%
.
@@ Coverage Diff @@
## master #51 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 8 8
Lines 200 211 +11
=====================================
+ Hits 200 211 +11
Impacted Files | Coverage Δ | |
---|---|---|
src/deprecated.jl | 100% <ø> (ø) |
:arrow_up: |
src/Memento.jl | 100% <ø> (ø) |
:arrow_up: |
src/handlers.jl | 100% <100%> (ø) |
:arrow_up: |
src/loggers.jl | 100% <100%> (ø) |
:arrow_up: |
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 407ab60...26db95b. Read the comment docs.
The argument around +
is that it fits with some other packages in the julia ecosystem (e.g., adding constraints in Convex.jl).
Convex uses that out of deference to MATLAB syntax used in its predecessors. Julia has many more options available including functions that mutate their arguments (+
was used in MATLAB because all MATLAB functions took arguments as copy-on-write).
Ahh, okay, I didn't realize that choice was based on MATLAB syntax. I did notice that having a shorthand for push!(logger, handler)
was nice when you have complicated handler constructors, but I suppose that isn't necessary for this PR. Maybe @omus has an opinion on this?
I'll note that several other languages have a shorthand for pushing elements onto a list:
swift: names += ["Fred"]
haskel: names ++ ["Fred"]
ruby: names << "Fred"
ocaml: names@["Fred"]
julia: names <! "Fred"
?
I think we can just stick with push!
since a mutating +=
is a little weird (I was alright with it originally since Convex.jl was doing it). I think having a shorthand would be nice but I we can open an issue and wait until we come up with something solid.
@iamed2 and @omus Is this good to merge?
Closes #31
Main changes include:
_
from public method names!
to mutating methodsadd_handler
andremove_handler
in favour ofpush!(logger, handler)
. I'll be replacing the internal handlerDict
with aVector
in a future release.+=
for adding handlers and filters to loggers (or filters to handlers).