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

Simple fix for buildlib python bug #241

Merged
merged 4 commits into from
Aug 21, 2023
Merged

Conversation

adrifoster
Copy link
Contributor

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

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix, @adrifoster !

@billsacks
Copy link
Member

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

@billsacks
Copy link
Member

@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

@jedwards4b jedwards4b self-requested a review August 21, 2023 18:55
@adrifoster
Copy link
Contributor Author

@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
Copy link
Member

What you have now is good - thanks.

@billsacks billsacks merged commit 4a6b374 into ESCOMP:main Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants