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

branch to help with pr review #672

Closed
wants to merge 1 commit into from

Conversation

jedwards4b
Copy link
Contributor

Branch for PR review of #640 only, not intended to be merged
Test suite:
Test baseline:
Test namelist changes:
Test status: [bit for bit, roundoff, climate changing]

Fixes [CIME Github issue #]

User interface changes?:

Code review:

if schema is not None:
# not checking schema on external components yet
cimeroot = get_cime_root()
if cimeroot in os.path.abspath(infile):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only applies to xml files within cime for now, when the component models catch up we can apply to them as well.

@@ -43,11 +40,12 @@ def get_valid_model_components(self):
components = comps.split(',')
return components

def _get_value_match(self, node, attributes=None):
def _get_value_match(self, node, attributes=None, exact_match=False):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Matching for components and compsets is regex matching, while matching for namelists is exact.

return default.text
else:
return None
def _get_node_element_info(self, node, element_name):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

General method to get text from a child element of node

@@ -125,7 +125,8 @@ def get_nodes(self, nodename, attributes=None, root=None, xpath=None):

for key, value in attributes.iteritems():
if value is not None:
expect(isinstance(value, str), " Bad value passed for key %s"%key)
expect(isinstance(value, str) or isinstance(value, unicode),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've since learned that this can be written isinstance(value, basestring), I will change this.

"""Interface to `namelist_definition.xml`.

This module contains only one class, `NamelistDefinition`, inheriting from
`GenericXML`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inherits from EntryID - I will fix.



def _get_version(self):
version = self.root.get("version")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

version 1 files may not have a version tag, i will fix.

@@ -67,6 +67,8 @@ def parse_namelists(namelist_lines, filename):
comment_re = re.compile(r'^[#!]')
namelist_re = re.compile(r'^&(\S+)$')
name_re = re.compile(r"^([^\s=']+)\s*=\s*(.+)$")
#name_re = re.compile(r"^([^\s=':]+)\s*=\s*(.+)$")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will remove

_ymd_re = re.compile(r"%(?P<digits>[1-9][0-9]*)?y(?P<month>m(?P<day>d)?)?")

_stream_file_template = """
<dataSource>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is some psuedo xml code for generating stream files, the plan is to eventually get rid of this style of input and put streams into namelist files.

@@ -21,8 +21,9 @@ def expect(condition, error_msg, exc_type=SystemExit):
"""
if (not condition):
# Uncomment these to bring up a debugger when an expect fails
#import pdb
#pdb.set_trace()
if logger.isEnabledFor(logging.DEBUG):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got tired of commenting or uncommenting these lines. Now if you run with --debug they are enabled but not otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I also uncomment these lines frequently. I think this is a reasonable change; it might be better to add another standard option to control this, but I think this is fine for now. You may want to remove the "Uncomment these ..." comment above.

from_dir=cimeroot).splitlines())
#TODO - get rid of this
list_of_directories_to_ignore = ("scripts/Tools", "point_clm", "tools", "machines", "apidocs", "unit_test")
for file_ in files_to_test:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes each pylint file test a separate test in scripts_regression_tests and extends it to include the
scripts directory and the buildnml and buildexe from driver and component models. The list_of_directories_to_ignore is a todo list of files to update to conform to pylint requirements.

@jedwards4b
Copy link
Contributor Author

Annotation complete, enjoy!

@@ -21,8 +21,9 @@ def expect(condition, error_msg, exc_type=SystemExit):
"""
if (not condition):
# Uncomment these to bring up a debugger when an expect fails
#import pdb
#pdb.set_trace()
if logger.isEnabledFor(logging.DEBUG):
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I also uncomment these lines frequently. I think this is a reasonable change; it might be better to add another standard option to control this, but I think this is fine for now. You may want to remove the "Uncomment these ..." comment above.

@jedwards4b jedwards4b closed this Oct 14, 2016
@jedwards4b jedwards4b deleted the fake_pr_for_review branch October 14, 2016 18:56
billsacks pushed a commit to billsacks/cime that referenced this pull request Mar 6, 2020
* Fix HTML search not working

Since Sphinx-1.7, Sphinx has exported search options as
`documentation_options.js`.  This starts to refer it instead of
definitions of JavaScript variables.
In addition, this also uses `js_tag()` function which added in 1.8
to support additional attributes for scripts (ex. async attribute).

* Add code comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants