Script to generate statvar dcid from constraint property and values#2093
Script to generate statvar dcid from constraint property and values#2093ajaits wants to merge 17 commits into
Conversation
…lbacks - Add strip_overlapping_prop_prefix to eliminate redundant property prefixes in DCIDs - Implement max_len fallbacks (datacommonsorg#3 raw code retry and datacommonsorg#4 cumulative dropping + MD5 hash) - Precompile module-level regular expressions for performance - Expand unit tests to 13 passing methods and enforce 80-column style limits
There was a problem hiding this comment.
Code Review
This pull request introduces automation scripts and templates for processing UN codelist and DSD files, including utilities to generate statvar groups, names, and custom DCIDs. Key feedback highlights several critical issues: a typo (lgging) and undefined variable (pvs) in generate_codelist_map.py, potential IndexError exceptions in string utility functions, a positional argument mismatch when calling generate_dcid_for_statvar in stat_var_processor.py, a duplicate dictionary key in config_flags.py, and a formatting bug in utils.py where extremely large numbers can still be incorrectly processed as integers.
| except Exception as e: | ||
| lgging.error( | ||
| f'Failed to evaluate "{tpl_val}" using dict: {pvs}, error:{e}') | ||
| value = '' |
There was a problem hiding this comment.
There is a typo lgging instead of logging, and pvs is undefined (it should be input_pvs). This will cause a NameError when an exception is caught during statement evaluation.
| except Exception as e: | |
| lgging.error( | |
| f'Failed to evaluate "{tpl_val}" using dict: {pvs}, error:{e}') | |
| value = '' | |
| except Exception as e: | |
| logging.error( | |
| f'Failed to evaluate "{tpl_val}" using dict: {input_pvs}, error:{e}') | |
| value = '' |
| if self._config.get('statvar_dcid_fixed_properties'): | ||
| # Use the custom statvar dcid generator | ||
| dcid = generate_dcid_for_statvar(pvs, self._config, | ||
| self._counters) | ||
| else: |
There was a problem hiding this comment.
The self._counters argument is passed as the third positional argument, which corresponds to schema_nodes in generate_dcid_for_statvar's signature. Since self._counters is not a dict, it fails the type check and is ignored, while the counters parameter remains None. You should pass self._counters as a keyword argument counters=self._counters.
| if self._config.get('statvar_dcid_fixed_properties'): | |
| # Use the custom statvar dcid generator | |
| dcid = generate_dcid_for_statvar(pvs, self._config, | |
| self._counters) | |
| else: | |
| if self._config.get('statvar_dcid_fixed_properties'): | |
| # Use the custom statvar dcid generator | |
| dcid = generate_dcid_for_statvar(pvs, self._config, | |
| counters=self._counters) |
| if abs(number) > max_int: | ||
| # Convert very large ints to float with potential loss of precision | ||
| number = float(number) | ||
| elif int(number) == number: | ||
| number_int = int(number) | ||
| return f'{number_int}' | ||
| # Return float rounded to precision digits. |
There was a problem hiding this comment.
If abs(number) > max_int, the number is converted to a float. However, because int(number) == number can still evaluate to True for large floats (e.g., int(1e25) == 1e25), the code will still enter the elif block, converting the float back to an int and returning a potentially imprecise integer string (e.g., '10000000000000000905969664'). If the number exceeds max_int, it should be formatted and returned as a float directly.
| if abs(number) > max_int: | |
| # Convert very large ints to float with potential loss of precision | |
| number = float(number) | |
| elif int(number) == number: | |
| number_int = int(number) | |
| return f'{number_int}' | |
| # Return float rounded to precision digits. | |
| if abs(number) > max_int: | |
| # Convert very large ints to float with potential loss of precision | |
| number = float(number) | |
| if precision_digits is not None: | |
| number = round(number, precision_digits) | |
| return f'{number}' | |
| elif int(number) == number: | |
| number_int = int(number) | |
| return f'{number_int}' |
| def to_dcid(code: str) -> str: | ||
| """Replace any non alphanumeric characters with '_'""" | ||
| value = re.sub(r'[^A-Za-z0-9\._:-]+', '_', code) | ||
| return value[0].upper() + value[1:] |
There was a problem hiding this comment.
If code is empty or becomes empty after regex substitution, value[0] will raise an IndexError. Adding a guard check for empty strings makes the function more robust against missing or empty codes in the input CSV.
| def to_dcid(code: str) -> str: | |
| """Replace any non alphanumeric characters with '_'""" | |
| value = re.sub(r'[^A-Za-z0-9\._:-]+', '_', code) | |
| return value[0].upper() + value[1:] | |
| def to_dcid(code: str) -> str: | |
| """Replace any non alphanumeric characters with '_'""" | |
| if not code: | |
| return '' | |
| value = re.sub(r'[^A-Za-z0-9\._:-]+', '_', code) | |
| return value[0].upper() + value[1:] if value else '' |
| def to_property(concept: str) -> str: | ||
| """Returns a property for the concept.""" | ||
| c = eval_functions.str_to_camel_case(concept.lower().replace('_', ' ')) | ||
| return c[0].lower() + c[1:] |
There was a problem hiding this comment.
If concept is empty or str_to_camel_case returns an empty string, c[0] will raise an IndexError. Adding a guard check ensures safety against empty or invalid concept names.
| def to_property(concept: str) -> str: | |
| """Returns a property for the concept.""" | |
| c = eval_functions.str_to_camel_case(concept.lower().replace('_', ' ')) | |
| return c[0].lower() + c[1:] | |
| def to_property(concept: str) -> str: | |
| """Returns a property for the concept.""" | |
| if not concept: | |
| return '' | |
| c = eval_functions.str_to_camel_case(concept.lower().replace('_', ' ')) | |
| return c[0].lower() + c[1:] if c else '' |
| # Settings for statvar dcid generator | ||
| 'statvar_dcid_fixed_properties': [], | ||
| 'statvar_dcid_prefix': | ||
| _FLAGS.statvar_dcid_prefix, | ||
| 'statvar_dcid_remove_prefix': | ||
| '', | ||
| 'statvar_dcid_delimiter': | ||
| '', | ||
| 'statvar_dcid_value_delimiter': | ||
| '', | ||
| 'statvar_dcid_upper_case': | ||
| False, | ||
| 'statvar_dcid_remove_prefix': | ||
| '', | ||
| } |
There was a problem hiding this comment.
The key 'statvar_dcid_remove_prefix' is duplicated in the dictionary literal. You should remove the duplicate entry.
| # Settings for statvar dcid generator | |
| 'statvar_dcid_fixed_properties': [], | |
| 'statvar_dcid_prefix': | |
| _FLAGS.statvar_dcid_prefix, | |
| 'statvar_dcid_remove_prefix': | |
| '', | |
| 'statvar_dcid_delimiter': | |
| '', | |
| 'statvar_dcid_value_delimiter': | |
| '', | |
| 'statvar_dcid_upper_case': | |
| False, | |
| 'statvar_dcid_remove_prefix': | |
| '', | |
| } | |
| # Settings for statvar dcid generator | |
| 'statvar_dcid_fixed_properties': [], | |
| 'statvar_dcid_prefix': | |
| _FLAGS.statvar_dcid_prefix, | |
| 'statvar_dcid_remove_prefix': | |
| '', | |
| 'statvar_dcid_delimiter': | |
| '', | |
| 'statvar_dcid_value_delimiter': | |
| '', | |
| 'statvar_dcid_upper_case': | |
| False, |
…tionary key in config_flags
Used for UN imports where dcid contains the statvar constraints