Unitful.string is actually Base.string

[sostock]

This package extends Unitful.string. But Unitful does not define a string function, so it actually extends Base.string. Calling it Unitful.string just seems confusing to me. Is there a reason for doing it like this?

It also means that this package would break if an actual Unitful.string function is defined in Unitful (the function in this package would then no longer extend Base.string but instead Unitful.string, which might have a different purpose and might not even be exported).

Also, irrespective of whether extending Unitful.string or Base.string, this is type piracy. It might be better to define a UnitfulParsableString.string function. It could have a fallback method that calls Base.string so that it can be used as a replacement. Of course, this would be a breaking change.

What do you think of this?

[michikawa07]

Thank you for your feedback, @sostock.

Indeed, since Unitful.jl does not define a string method, it effectively means that this pkg is extending Base.string, not Unitful.string. Originally, this pkg was intended to include string methods that I thought should be part of the main Unitful.jl package, which is why the naming might seem confusing.

I understand that the current string definition in this package constitutes type piracy.
However, I can't think of another way to override the conversion to string used in CSV output and other similar cases.
Even if we define UnitfulParsableString.string, we cannot change the string used in CSV.write to UnitfulParsableString.string.

Changing the current implementation to extend Base.string is easy and non-destructive, so I think that's a good short-term solution.
In the longer term, I believe the string methods in UnitfulParsableString.jl should be moved to Unitful.jl.
(In that case, it would likely involve multi-dispatching the show method instead of string, but the function's content would remain almost the same.)
I'm working on this development, but due to time constraints, I haven't been able to submit a PR. I apologize for the delay.
(The implemention of PainterQubits/Unitful.jl/#214)

Conversely, I have a question: other than the above, do you plan to change the string or show methods in Unitful?

[sostock]

I understand that the current string definition in this package constitutes type piracy. However, I can't think of another way to override the conversion to string used in CSV output and other similar cases.

Yes, type piracy is the only way to get something like this working without incorporating it in Unitful.jl. And I don’t think the type piracy is too bad in this case. I mainly dislike the naming Unitful.string.

Changing the current implementation to extend Base.string is easy and non-destructive, so I think that's a good short-term solution.

Yes, I think this should be done as it is less confusing and would have no effect on existing code that uses this package.

Conversely, I have a question: other than the above, do you plan to change the string or show methods in Unitful?

I currently have no plans to change the string or show methods in Unitful, but I am generally in favor of changing the default show output to be parseable. The implementation in this package could be used for that (but extending show instead of string).

[michikawa07]

Thanks for the reply.

I just changed Unitful.string to Base.string.

I currently have no plans to change the string or show methods in Unitful, but I am generally in favor of changing the default show output to be parseable. The implementation in this package could be used for that (but extending show instead of string).

OK, thanks! As soon as I finish the doctoral course, I will try this one.