Code review comment for lp:~jon-hill/fluidity/diagnostic_check

Revision history for this message
Stephan Kramer (s-kramer) wrote :

Ad 1) Fine, but in all other Fluidity code ALLCAPS is only used for parameters. If do loop labels are hard to find, it means that the loop is too long (as in this case: the code under both cases are a copy and paste, so that should really just be separated out in a subroutine)

Ad 2) For the relative paths you have "scalar_field::" // trim(split_dependency(1)) everywhere, where split_dependency(1) is a phase name. For the absolute paths, I'm not really sure it makes sense for the user to specify PhaseName::DistanceToTop as the user has no clue that these fields are actually put in one of the (or in this case all) states, so they should just refer to it as DistanceToTop. If you want to allow it however I have no objections to that.

« Back to merge proposal