-
Notifications
You must be signed in to change notification settings - Fork 64
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
Fix/report Python3 decoding errors, parse all suites if --suites=... argument not present #292
Fix/report Python3 decoding errors, parse all suites if --suites=... argument not present #292
Conversation
… decoding non-ascii files in scripts/metadata_parser.py, parse all suite definition files if suites argument is not present when calling ccpp_prebuild.py
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.
Not sure about some of the design decisions but seems okay.
def get_all_suites(suites_dir): | ||
success = False | ||
logging.info("No suites were given, compiling a list of all suites") | ||
sdfs = [] | ||
for f in os.listdir(suites_dir): | ||
match = SUITE_DEFINITION_FILENAME_PATTERN.match(f) | ||
if match: | ||
logging.info('Adding suite definition file {}'.format(f)) | ||
sdfs.append(f) | ||
if sdfs: | ||
success = True | ||
return (success, sdfs) | ||
|
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.
This seems like a lot of work to do instead of something like:
sdfs = glob.glob(suite_dir, "suite_*.xml")
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.
That's right ... when I was making those changes I wasn't sure whether I needed to get the suite name (which comes for free as match.group(1)
with my approach) or not. Turned out I didn't.
In any case, even the more expensive approach costs probably less than a tenth of a second, so that should be ok.
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.
This looks good to me. Approved.
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.
approved.
@JulieSchramm @ligiabernardet the changes in this PR require an update of the technical documentation ... looking for volunteers ... |
We will get to this but it will be a couple of weeks. We will need to ask
some questions before we get started.
…On Wed, May 13, 2020 at 1:50 PM Dom Heinzeller ***@***.***> wrote:
@JulieSchramm <https://github.com/JulieSchramm> @ligiabernardet
<https://github.com/ligiabernardet> the changes in this PR require an
update of the technical documentation ... looking for volunteers ...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#292 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE7WQAUW6UY3KS2BNSBYISLRRL2ZLANCNFSM4M6JPOLA>
.
|
This PR:
scripts/common.py
scripts/metadata_parser.py
ccpp_prebuild.py
; this addresses issue CCPP code generator needs to parse all available suites if SUITES argument is not present #293Associated PRs:
NOAA-EMC/GFDL_atmos_cubed_sphere#20
#292
NCAR/ccpp-physics#451
NOAA-EMC/fv3atm#115
NOAA-EMC/NEMS#62
ufs-community/ufs-weather-model#126
For regression testing information, see ufs-community/ufs-weather-model#126.