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

Implement objects to represent YAML versions #223

Open
Paalon opened this issue Jun 20, 2024 · 8 comments
Open

Implement objects to represent YAML versions #223

Paalon opened this issue Jun 20, 2024 · 8 comments

Comments

@Paalon
Copy link
Contributor

Paalon commented Jun 20, 2024

#198 (comment)_

@Paalon
Copy link
Contributor Author

Paalon commented Jun 20, 2024

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

@GunnarFarneback
Copy link
Contributor

GunnarFarneback commented Jun 20, 2024

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
Copy link
Contributor

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.

@Paalon
Copy link
Contributor Author

Paalon commented Jun 20, 2024

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
Copy link
Contributor Author

Paalon commented Jun 20, 2024

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

@GunnarFarneback
Copy link
Contributor

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.

@Paalon
Copy link
Contributor Author

Paalon commented Jun 21, 2024

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
Copy link
Contributor Author

Paalon commented Jun 21, 2024

Let's use abstract type & subtyping pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants