LibSerialPort.jl icon indicating copy to clipboard operation
LibSerialPort.jl copied to clipboard

Type piracy with `Base.open`

Open dawbarton opened this issue 5 years ago • 5 comments

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.

dawbarton avatar Nov 17 '20 16:11 dawbarton

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 avatar Nov 17 '20 16:11 mgkuhn

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 avatar Nov 17 '20 16:11 mgkuhn

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).

mgkuhn avatar Nov 17 '20 16:11 mgkuhn

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).

dawbarton avatar Nov 17 '20 16:11 dawbarton

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

jebej avatar Dec 08 '22 14:12 jebej