-
Notifications
You must be signed in to change notification settings - Fork 21
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
Give (non-type) global constants upper case names #534
base: master
Are you sure you want to change the base?
Conversation
Float is a type though right? |
Ah right
Then I think indeed it can be removed, but that might require some refactoring of type parameters |
Yeah I made a separate issue, #535. I'm fine with renaming it as well in this PR. |
Yes, I agree this can be removed, for me it's fine to do this as part of this PR. |
The wflow server tests are failing now.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Two small comments, one related to the failing wflow server tests.
@@ -72,7 +71,8 @@ instates_piave_gwf_path = | |||
|
|||
include("testing_utils.jl") | |||
|
|||
@info "testing Wflow with" nthreads() VERSION Float | |||
FLOAT = Float64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to remove FLOAT
here, as this setting has been removed from Wflow.jl.
@@ -85,35 +85,35 @@ struct GetVarLocation | |||
name::String | |||
end | |||
|
|||
struct GetValue | |||
struct GetValue{T} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need parametric type structs here? The tests are failing now because the JSON message with the dest
entry is now mapped to Vector{Any}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah indeed I also suggested to @SouthEndMusic to just remove them for now. If we want to support parametric precision we should probably do that in a separate PR and think about which structs would need type parameters.
My apologies for the many PR's, but this is a very simple one. All I did was give (non-type) global constants upper case names. This helps with readability of the code for people who are not familiar with all the variable names.
Edit: fixes #535