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 (set|with)context to bundle arguments to IOContext #14

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

fredrikekre
Copy link
Contributor

Implement IOContextThingy (placeholder) to bundle arguments to IOContext with an object.

@fredrikekre fredrikekre changed the title Implement IOContextThingy (placeholder) to bundle Implement IOContextThingy (placeholder) Nov 30, 2020
@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #14 (0d8b9f7) into master (9594fbe) will increase coverage by 41.75%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #14       +/-   ##
===========================================
+ Coverage   42.85%   84.61%   +41.75%     
===========================================
  Files           1        1               
  Lines           7       13        +6     
===========================================
+ Hits            3       11        +8     
+ Misses          4        2        -2     
Impacted Files Coverage Δ
src/DisplayAs.jl 84.61% <88.88%> (+41.75%) ⬆️

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 9594fbe...0d8b9f7. Read the comment docs.

@tkf
Copy link
Owner

tkf commented Dec 1, 2020

This totally makes sense, but I have no idea how to name the IOContextThingy. Hmm.... let's call it IOContextCarrier unless you have other ideas.

@fredrikekre
Copy link
Contributor Author

Updated to IOContextCarrier.

@fredrikekre
Copy link
Contributor Author

bump

@mortenpi
Copy link

mortenpi commented Aug 1, 2021

Just throwing two ideas out here:

  • I often use the other DisplayAs objects with |>, e.g.

    myplot |> DisplayAs.PNG

    So it might be nice if we could use IOContextCarrier the same way:

    myobj |> IOContextCarrier(:limit => 2)

    That would require something like IOContextCarrier(kv::Pair...) = (x -> IOContextCarrier(x, kv...)), although that would conflict with the current method I think. Could IOContextCarrier use keyword arguments, instead of Pairs?

  • Bikeshedding the name: DisplayAs.IOContextCarrier is pretty long. Maybe DisplayAs.IOCtx? Or, if we'd want to be evil, it could also be DisplayAs.IOContext -- I think the is kind of a precedent with DisplayAs.HTML?

@fredrikekre
Copy link
Contributor Author

Could also be implemented as a callable struct then instead of returning an anonymous function. I don't mind DisplayAS.IOContext, what do you think @tkf?

@tkf
Copy link
Owner

tkf commented Sep 16, 2021

Thanks for the ping. Sorry, forgot about this PR. Yes, I agree IOContextCarrier is rather long and not super pretty. But I worry that DisplayAs.IOContext can be confusing since its instance is not really an IOContext (and IO).

Actually, we don't need to use the type names as API. How about

DisplayAs.setcontext(obj, kvs::Pair...) = IOContextCarrier(obj, kvs...)
DisplayAs.withcontext(kvs::Pair...) = obj -> IOContextCarrier(obj, kvs...)

(where IOContextCarrier is just an internal type)? I want to separate out the curried version as it's reasonable to want to use this for IO-contextualizing a Pair.

@fredrikekre
Copy link
Contributor Author

fredrikekre commented Jan 24, 2022

I implemented the proposed changes. CI config is outdated here, do you want me to update that? Edit: #15

@fredrikekre fredrikekre changed the title Implement IOContextThingy (placeholder) Implement (set|with)context to bundle arguments to IOContext Jan 24, 2022
@tkf
Copy link
Owner

tkf commented Jan 24, 2022

Thanks a lot for your patience! And sorry for the very slow process.

@fredrikekre fredrikekre deleted the fe/ioc branch January 24, 2022 21:54
@fredrikekre
Copy link
Contributor Author

fredrikekre commented Jan 24, 2022

No worries. Can you make a new release too? Thanks. Edit: Just saw JuliaRegistries/General#53113, thanks.

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.

None yet

3 participants