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

Make keyword argument names consistent #27

Closed
mroavi opened this issue Apr 27, 2022 · 10 comments · Fixed by #45
Closed

Make keyword argument names consistent #27

mroavi opened this issue Apr 27, 2022 · 10 comments · Fixed by #45
Labels
question Further information is requested

Comments

@mroavi
Copy link

mroavi commented Apr 27, 2022

For consistency, I propose changing this constructor signature from:

MetaGraphsNext.MetaGraph(graph::AbstractGraph{T}; Label, VertexData, EdgeData, graph_data, weight_function, default_weight) where T 

to

MetaGraphsNext.MetaGraph(graph::AbstractGraph{T}; label, vertex_data, edge_data, graph_data, weight_function, default_weight) where T 
@mroavi
Copy link
Author

mroavi commented Apr 27, 2022

Hmm. I think I understand now. The pascal case is being used to distinguish arguments that expect a type. This wasn't obvious to me. I haven't seen this convention in other Julia packages. What about making it a bit more verbose but more clear at the same time:

MetaGraphsNext.MetaGraph(graph::AbstractGraph{T}; label_type, vertex_data_type, edge_data_type, graph_data, weight_function, default_weight) where T 

@bramtayl
Copy link
Collaborator

Yup, I came up with that convention. I kind of like it; vertex_data_type seems a bit long for a keyword. But I don't have strong opinions about it.

@mroavi
Copy link
Author

mroavi commented Apr 29, 2022

Would be nice to hear other people's opinions. In my opinion, it breaks an important styling convention advocated in Julia's Base lib where variables (arguments in this case) are defined in lower case and actual types (or modules) in upper case.

@gdalle
Copy link
Member

gdalle commented Feb 22, 2023

I don't have strong opinions either but I don't really like that constructor anyway, since it is not inferrable. I think #6 will be part of the answer

@gdalle gdalle added the question Further information is requested label Feb 22, 2023
@bramtayl
Copy link
Collaborator

it is not inferrable

Oof I didn't realize that. Even with constant propagation of the types? That seems like arguably a bug in base.

@gdalle
Copy link
Member

gdalle commented Feb 22, 2023

Oof I didn't realize that. Even with constant propagation of the types? That seems like arguably a bug in base.

I think types as kwargs are never inferrable, are they? Anyway, working on fixing it with a new constructor for upcoming v0.5

@bramtayl
Copy link
Collaborator

types as kwargs are never inferrable

This seems to work?

using Test: @inferred

function keyword_function(; key_type = Symbol, value_type = Int)
    return Dict{key_type, value_type}()
end

function wrapper_function()
    keyword_function(; key_type = String)
end

@inferred keyword_function()
@inferred wrapper_function()

@gdalle
Copy link
Member

gdalle commented Feb 22, 2023

It does but this fails for instance

julia> @inferred keyword_function(; key_type=String)
ERROR: return type Dict{String, Int64} does not match inferred return type Dict{_A, Int64} where _A
Stacktrace:
   [1] error(s::String)
     @ Base ./error.jl:35

I think it's the same topic as this recent discourse thread https://discourse.julialang.org/t/keyword-arguments-and-type-stability/94993

@bramtayl
Copy link
Collaborator

Oh, right sure, that's not with constant propagation though.

@gdalle
Copy link
Member

gdalle commented Feb 25, 2023

I documented it in more detail in #45

@gdalle gdalle closed this as completed in #45 Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants