JuliaIO / LibSerialPort.jl

Julia wrapper for the libserialport c library
MIT License
65 stars 21 forks source link

Type piracy with `Base.open` #79

Open dawbarton opened 3 years ago

dawbarton commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

mgkuhn commented 3 years ago

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 commented 3 years ago

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 commented 1 year ago

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