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

Implement objects to represent YAML versions

Open Paalon opened this issue 1 year ago • 8 comments

https://github.com/JuliaData/YAML.jl/pull/198#discussion_r1643573887_

Paalon avatar Jun 20 '24 02:06 Paalon

Which is better?

abstract type YAMLVersion end

struct YAMLV1_1 <: YAMLVersion end

struct YAMLV1_2 <: YAMLVersion end

forwardchars!(::YAMLV1_1, stream::TokenStream, n::Integer=1) #...
struct YAMLVersion{T} end

const YAMLV1_1 = YAMLVersion{:Version1_1}()

const YAMLV1_2 = YAMLVersion{:Version1_2}()

forwardchars!(::YAMLV1_1, stream::TokenStream, n::Integer=1) #...

Paalon avatar Jun 20 '24 02:06 Paalon

I'm clearly finding the PR and the issue in the wrong order here.

As I see it the biggest difference is that YAMLV1_1 is a type in the first case and an instance in the second, with the consequence that calls become

f(YAMLV1_1(), ...)

vs

f(YAMLV1_1, ...)

whereas definitions become

f(::YAMLV1_1, ...) = ...

vs

f(::YAMLVersion{:Version1_1}, ...) = ...

Functionally it shouldn't make a difference so I'd vote for the first being better on the grounds that the calls and definitions are more uniform, plus having to write and read the long form of the second in definitions seems worse.

GunnarFarneback avatar Jun 20 '24 12:06 GunnarFarneback

The fact that you won't get a compiler error from misspelling

f(::YAMLVersion{:Verion1_1}, ...) = ...

is also an argument in favor of the first option.

GunnarFarneback avatar Jun 20 '24 12:06 GunnarFarneback

Year, the latter should be

struct YAMLVersion{T} end

const YAMLV1_1 = YAMLVersion{:Version1_1}()

const YAMLV1_2 = YAMLVersion{:Version1_2}()

# definition
forwardchars!(::YAMLVersion{:Version1_1}, stream::TokenStream, n::Integer=1) #...

# call
forwardchars!(YAMLV1_1, stream)

We can see this for example in Julia Base's RoundingMode:

https://github.com/JuliaLang/julia/blob/a14cc38512b6daab6b8417ebb8a64fc794ff89cc/base/rounding.jl#L48

Paalon avatar Jun 20 '24 18:06 Paalon

I have asked this in Discourse: Which is better for traits? Subtyping vs parametric typing

Paalon avatar Jun 20 '24 18:06 Paalon

I believe RoundingMode is designed that way because the instances are part of the public API for the involved functions. Furthermore those functions are so small and fast that any calling overhead would be disastrous.

GunnarFarneback avatar Jun 20 '24 20:06 GunnarFarneback

I asked this also in Julia Slack's internal channel. Simon Byrne said

I think that’s my fault. Not sure I would make it parametric if I were to do it again

So there seems to be no specific reason to that implementation.

Paalon avatar Jun 21 '24 05:06 Paalon

Let's use abstract type & subtyping pattern.

Paalon avatar Jun 21 '24 05:06 Paalon