ESCOMP / CDEPS

Community Data Models for Earth Prediction Systems
https://escomp.github.io/CDEPS/versions/master/html/index.html
18 stars 39 forks source link

Simple fix for buildlib python bug #241

Closed adrifoster closed 11 months ago

adrifoster commented 11 months ago

Description of changes

As laid out in #240 a bug was found in the buildlib script

This code update changes the append line to a string concatenation and updates the if "F90" in line and not "fox" in line: line to ensure different capitalizations of "fox" don't slip through.

Contributors other than yourself, if any: @billsacks

CDEPS Issues Fixed (include github issue #): #240

Are there dependencies on other component PRs (if so list): No

Are changes expected to change answers (bfb, different to roundoff, more substantial): No

Any User Interface Changes (namelist or namelist defaults changes): No

Testing performed (e.g. aux_cdeps, CESM prealpha, etc):

Tested manually with SMS_D_Ld1_P8x1.f10_f10_mg37.I2000Clm50BgcCropQianRs.fleabone_gnu.clm-default (on personal computer)

Hashes used for testing: cdeps1.0.13-1-gd31de60

billsacks commented 11 months ago

@jedwards4b - do you want to look at this before it's merged?

billsacks commented 11 months ago

@adrifoster - A minor issue with your latest commit is that (I think) the error message will use the all-lowercase version of the line, which may be harder to read / interpret. If your goal is to avoid the duplication of line.lower(), maybe you could do something like:

for line in e.split("\n"):
    line_lower = line.lower()
    if "f90" in line_lower and not "fox" in line_lower:
        nextline = nextline + line
adrifoster commented 11 months ago

@adrifoster - A minor issue with your latest commit is that (I think) the error message will use the all-lowercase version of the line, which may be harder to read / interpret. If your goal is to avoid the duplication of line.lower(), maybe you could do something like:

for line in e.split("\n"):
    line_lower = line.lower()
    if "f90" in line_lower and not "fox" in line_lower:
        nextline = nextline + line

Whoops! That is a good point. I really did it to just make it look nicer, so I put it back. Though if you think your version is better, happy to change it to that.

billsacks commented 11 months ago

What you have now is good - thanks.