Skip to content
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

implement Base.length for Record #165

Merged
merged 1 commit into from
Aug 27, 2020
Merged

Conversation

KristofferC
Copy link
Contributor

This makes test pass on Julia master. It also allows showing Records in e.g.
the REPL while previously it would error with:

julia> rec = DefaultRecord("Logger.example", "info", 20, "blah")
Error showing value of type DefaultRecord:
ERROR: MethodError: no method matching length(::DefaultRecord)
Closest candidates are:
  length(::Cmd) at process.jl:639
  length(::CompositeException) at task.jl:41
  length(::Base.MethodList) at reflection.jl:869
  ...
Stacktrace:
 [1] summary(::IOContext{REPL.Terminals.TTYTerminal}, ::DefaultRecord) at ./abstractdict.jl:34

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #165 into master will increase coverage by 1.54%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
+ Coverage   95.19%   96.73%   +1.54%     
==========================================
  Files          13       13              
  Lines         333      337       +4     
==========================================
+ Hits          317      326       +9     
+ Misses         16       11       -5     
Impacted Files Coverage Δ
src/records.jl 86.66% <0.00%> (-2.62%) ⬇️
src/syslog.jl 66.66% <0.00%> (-33.34%) ⬇️
src/exceptions.jl 50.00% <0.00%> (-16.67%) ⬇️
src/loggers.jl 99.09% <0.00%> (+0.90%) ⬆️
src/handlers.jl 100.00% <0.00%> (+5.76%) ⬆️
src/formatters.jl 98.03% <0.00%> (+5.88%) ⬆️
src/stdlib.jl 88.88% <0.00%> (+11.11%) ⬆️

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 ad32ea8...678883d. Read the comment docs.

@rofinn
Copy link
Member

rofinn commented Aug 27, 2020

Thanks, is this a new requirement of any subtype of AbstractDict?

@rofinn rofinn merged commit fd31660 into invenia:master Aug 27, 2020
@KristofferC
Copy link
Contributor Author

KristofferC commented Aug 27, 2020

Well, the problem is that HasLength is true by default and you didn't provide a length method so you were technically breaking the interface. Right now, the dictionary constructor will try to use the length method if HasLength is true for some performance win, which it didn't use to.

@KristofferC KristofferC deleted the kc/length branch August 27, 2020 16:14
@rofinn
Copy link
Member

rofinn commented Aug 27, 2020

Ahh, okay, thanks.

@KristofferC
Copy link
Contributor Author

Could there be a version bump with this in it?

@rofinn
Copy link
Member

rofinn commented Nov 18, 2020

Sorry, done. JuliaRegistries/General#24900

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.

2 participants