Skip to content

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.

4 participants