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

Type piracy with Base.open #79

Open
dawbarton opened this issue Nov 17, 2020 · 5 comments
Open

Type piracy with Base.open #79

dawbarton opened this issue Nov 17, 2020 · 5 comments

Comments

@dawbarton
Copy link

I was just adapting some of my code to use the newest release and I noticed that there appears to be some type piracy with extending Base.open. Specifically it gets extended with

[12] open(portname::AbstractString, baudrate::Integer; mode, ndatabits, parity, nstopbits) in LibSerialPort at C:\Users\db9052\.julia\packages\LibSerialPort\zyrgR\src\LibSerialPort.jl:456

which doesn't use any types defined by LibSerialPort (hence the piracy). While I can see that this is a convenience function (and currently doesn't cause problems), it's probably better not to do it since a string and an integer as inputs is quite generic.

An alternative might be to introduce a string macro/lightweight struct such that you can do

open(sp"/dev/ttyUSB0", 9600)

I'd be happy to add the required code if that would help.

@mgkuhn
Copy link
Collaborator

mgkuhn commented Nov 17, 2020

Thanks for raising this type piracy issue. I fully agree. We should not define a method open(::String,::Integer) of Base.open that does not have a type specific to this package as an argument, as that method could very easily collide with one from any other package that made the same mistake. Imagine, for example, a GPIB.jl package (for controlling devices on GPIB buses), which could just as well define another Base.open(::String,::Integer) method, where the String would be the bus identifier and the Integer would be the GPIB device address (rather than a baud rate).

That is actually the reason why I have always written LibSerialPort.open (see e.g. the example in ?LibSerialPort) instead of just open in the documentation, such that we can remove import Base: open and make open a separate function.

The same problem does not occur with Base.close or any of the other methods that start with a SerialPort argument. So open is really a special case.

@mgkuhn
Copy link
Collaborator

mgkuhn commented Nov 17, 2020

Regarding the sp"/dev/ttyUSB0" suggestion: I would not pollute the increasingly crowded ~1–3 letter namespace of string macros for such a minor reason.

I think the solution is not to extend Base.open at all, but to always require that users write either

  • LibSerialPort.open(...), to make it unambiguously clear from which module they want to open something, or to use a
  • SerialPort(...) constructor to get an open SerialPort object.

@mgkuhn
Copy link
Collaborator

mgkuhn commented Nov 17, 2020

We have just more cleanly separated the low-level API from the high-level API, and the next step will now have to be to polish the high-level API a bit more, such that it is self-sufficient (i.e., can be used without also having to use methods from the low-level API) and also to tidy it up and make sure it follows good Julia API design practice. Resolving this type piracy issue is part of that (but there are also others).

@dawbarton
Copy link
Author

Yes, I agree. There is already a suitable SerialPort constructor implemented, so it could just be a case deleting the open(::AbstractString, ::Integer) method. It's just a question of whether you want to keep it around for convenience (but not exported).

@jebej
Copy link

jebej commented Dec 8, 2022

I haven't checked, but it's likely that this causes invalidations too.

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

No branches or pull requests

3 participants