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

define IOContext(io, pairs...) and remove IOContext(io; kw...) #23271

Merged
merged 1 commit into from
Aug 24, 2017

Conversation

rfourquet
Copy link
Member

  • Usually a context is defined via a pair (IOContext(io, :k=>v)),
    but if another pair has to be pushed, one had to either add an
    IOContext call (IOContext(IOContext(io, :k=>v), :x=>y)) or to
    switch to kwargs syntax (IOContext(io, k=v, x=y)), none
    of which is satisfactory.
  • As a consequence of allowing passing multiple pairs, the
    method using keyword arguments is obsoleted (and hence deleted).
  • Also, the method IOContext(io, key, value) is deleted in
    favor of IOContext(io, key=>value).

I originally only wanted to allow IOContext(io, pairs...), but in the process saw opportunities for simplification of the API, as IOContext(io, :key, value) and IOContext(io, key=value) were almost not used. I have no problem to restore them if they are deemed necessary.

@rfourquet rfourquet added the io Involving the I/O subsystem: libuv, read, write, etc. label Aug 15, 2017
@rfourquet
Copy link
Member Author

So a bit of history:

  • IOContext is introduced in 4706184;
  • the kewyord version was introduced in ce23174, apparently to construct more easily IOContext with more than 1 property;
  • in 893a65b, many keyword calls are converted into pair calls (including when there is more than one property! i.e. IOContext(io, a=1, b=2) transformed into IOContext(IOContext(io, :a=>1), :b=>2)), probably for better performance;
  • in 8db51e6, some pair calls are converted into keyword calls (not sure why).

In other words, having two concurrent APIs is kind of messy; as the prefered syle seems to use pairs, and it's faster (as of today), this confirms my impression that we should get rid of the keyword version, which can I think accomplish nothing more than the pairs version. So I added a deprecation, and I will remove the WIP.

As the main authors of IOContext, @vtjnash @JeffBezanson would you mind having a look?

@rfourquet rfourquet changed the title WIP: define IOContext(io, pairs...) and remove IOContext(io; kw...) define IOContext(io, pairs...) and remove IOContext(io; kw...) Aug 17, 2017
"""
IOContext(io::IO; properties...)
unwrapcontext(io::IO) = io, ImmutableDict{Symbol,Any}()
unwrapcontext(io::IOContext) = io.io, io.dict
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced these two methods as they often allow to merge two methods (for IO and for IOContext) into one; moreover, they make it easy to spot what is discarded (e.g. in IOContext(unwrapcontext(io)[1], unwrapcontext(context)[2]), only the stream of io and the dict of context are kept).

@fredrikekre
Copy link
Member

If you feel like fixing an issue while you are at it, there is #22637 to add some more examples :)

@rfourquet
Copy link
Member Author

If you feel like fixing an issue while you are at it, there is #22637 to add some more examples :)

OK, I can do that! but will use another PR.

@rfourquet
Copy link
Member Author

IOContext is described as being "an immutable dictionary that is a subclass of IO". The API for a dictionary is to use pairs, so I think this PR brings more consistency. I will merge in a couple of days if no objection.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is much nicer and more consistent.

* Usually a context is defined via a pair (`IOContext(io, :k=>v)`),
  but if another pair has to be pushed, one had to either add an
  IOContext call (`IOContext(IOContext(io, :k=>v), :x=>y)`) or to
  switch to kwargs syntax (`IOContext(io, k=v, x=y)`), none
  of which is satisfactory.
* As a consequence of allowing passing multiple pairs, the
  method using keyword arguments is obsoleted (and hence deprecated).
* Also, the method `IOContext(io, key, value)` is deprecated in
  favor of `IOContext(io, key=>value)`.
@rfourquet
Copy link
Member Author

Wow, it became rare to see CI all green :)

@rfourquet rfourquet merged commit b056235 into master Aug 24, 2017
@rfourquet rfourquet deleted the rf/IOContext-pairs branch August 24, 2017 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants