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

[Breaking] Remove the type piracy of several Base functions #749

Closed
wants to merge 1 commit into from
Closed

[Breaking] Remove the type piracy of several Base functions #749

wants to merge 1 commit into from

Conversation

DilumAluthge
Copy link
Member

This pull request removes the type piracy of several Base functions. Specifically, it removes the following pirated methods:

  1. Base.startwrite(io::Any)
  2. Base.closewrite(io::Any)
  3. Base.startread(io::Any)
  4. Base.closeread(io::Any)

According to #745 (comment), this is a breaking change. Therefore, I took advantage of the opportunity to bump the version number to 1.0.0.

For context, see: #745 (review) and JuliaLang/julia#42080.

@DilumAluthge
Copy link
Member Author

cc: @fredrikekre @KristofferC @quinnj

@DilumAluthge
Copy link
Member Author

@quinnj I'll probably need your help to fix the tests.

@c42f
Copy link
Contributor

c42f commented Sep 8, 2021

None of startwrite, startread, closeread are defined in Base yet and IIUC there's no particular plan to add them. (I could potentially see closeread being added for symmetry with closewrite in the future. Not so sure about startread/startwrite.)

From my point of view I like @quinnj's current solution better for closewrite, somewhat hacky as it may be. The point is that closewrite and the new Base.closewrite should have exactly the same semantics because they solve the same problem. Thus, they deserve to share method tables.

SemVer is a blunt tool as usual... but if we need to release a breaking change anyway, I'd suggest we figure out how to unify things rather than separate them.

@KristofferC
Copy link
Contributor

KristofferC commented Sep 8, 2021

The point is that closewrite and the new Base.closewrite should have exactly the same semantics because they solve the same problem. Thus, they deserve to share method tables.

Why should closewrite(io) = nothing exist? And in that case, it should be moved to Base.

@c42f
Copy link
Contributor

c42f commented Sep 8, 2021

Why should closewrite(io) = nothing exist? And in that case, it should be moved to Base.

Well I agree you're right about that! If this is a nonsensical method, we should just remove this fallback method from HTTP.jl and declare a breaking change.

What I disagree about is something subtler: this PR makes HTTP.closewrite and Base.closewrite into separate generic functions with disjoint method tables and clashing exported names. I don't think we should do this because they have the same semantics.

@fredrikekre fredrikekre added this to the 1.0 milestone Sep 29, 2021
@DilumAluthge DilumAluthge deleted the dpa/remove-type-piracy-base-closewrite branch November 9, 2021 03:17
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.

4 participants