Update logging with info on the source file running #531

sven-oly · 2025-09-09T20:22:05Z

Fixes #417
Fixes #432

Adds the name of the python file being executed in logging output.

Also removes old unneeded code.

Add npm command to handle package-lock.

gemini-code-assist

Summary of Changes

Hello @sven-oly, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on improving the system's observability and maintainability. It standardizes logging output by including the source file path, refines the build process by addressing npm package lock warnings, and removes obsolete test data generation code, leading to a cleaner and more efficient system.

Highlights

Enhanced Logging: Added file path prefixes to log messages across various Python scripts for better traceability and debugging.
Build Process Improvement: Introduced "npm install --package-lock-only" in shell scripts to manage "package-lock.json" and clear warnings, ensuring consistent dependency management.
Code Cleanup and Simplification: Removed deprecated Node.js-based test data generation logic from "testgen/generators/datetime_fmt.py" and other unneeded code, streamlining the codebase.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

sven-oly · 2025-09-09T20:23:22Z

#417 #432 #

gemini-code-assist

Code Review

This pull request aims to improve logging by adding the source file name to log messages. While this is a good goal, the current implementation hardcodes the filenames in log strings across many files, which poses a maintainability challenge. I've provided suggestions to dynamically determine the filename instead. The PR also includes some welcome code cleanup by removing unneeded sections.

I've identified two critical issues: a typo in a variable name in testdriver/testplan.py that will likely cause runtime errors, and a NameError in an exception handler in testgen/generators/message_fmt2.py. There are also some minor typos. Please review the detailed comments.

gemini-code-assist · 2025-09-09T20:24:50Z

testdriver/testplan.py

    # Send a single line of data or command to Stdout, capturing the output
    def send_one_line(self, input_line):
-        self.run_error_message = None
+        self.runer_ror_message = None


This appears to be a typo. The attribute should likely be self.run_error_message to be consistent with its usage elsewhere in this file. This change will probably lead to an AttributeError since other parts of the code refer to self.run_error_message.

Suggested change

self.runer_ror_message = None

self.run_error_message = None

gemini-code-assist · 2025-09-09T20:24:50Z

testgen/generators/message_fmt2.py

+                logging.error("testgen/generators/message_fmt2.py: JSON %s not validated against schem a %s. Error = %s",
+                              test_file_path, json_schema_path, error)


This log message has a couple of issues:

There's a typo: schem a should be schema.

More importantly, it uses a variable error which is not defined in this except block. The exception object is named err. This will cause a NameError when this exception is caught.

Suggested change

logging.error("testgen/generators/message_fmt2.py: JSON %s not validated against schem a %s. Error = %s",

test_file_path, json_schema_path, error)

logging.error("testgen/generators/message_fmt2.py: JSON %s not validated against schema %s. Error = %s",

test_file_path, json_schema_path, err)

gemini-code-assist · 2025-09-09T20:24:50Z

testgen/generators/base.py

                return json.load(testdata) if filetype == "json" else testdata.read()
        except BaseException as err:
-            logging.warning("** readFile: %s", err)
+            logging.warning("testgen/generators: %s", err)


Hardcoding the path prefix 'testgen/generators' in this log message is not ideal for maintainability. It would be better to use __name__ or __file__ to dynamically get module or file information for logging.

gemini-code-assist · 2025-09-09T20:24:51Z

testdriver/testdriver.py


            if test_type not in ddt_data.testDatasets:
-                logging.warning('**** WARNING: test_type %s not in testDatasets', test_type)
+                logging.warning('testdriver.py **** WARNING: test_type %s not in testDatasets', test_type)


Hardcoding the filename prefix in log messages, as done throughout this file, is not ideal for maintainability. If the file is ever renamed, all these log strings would need to be updated manually. A better approach is to obtain the filename dynamically. You can define a constant at the top of the module:

import os FILENAME = os.path.basename(__file__)

And then use it in your log messages, for example:
logging.warning('%s **** WARNING: test_type %%s not in testDatasets', FILENAME, test_type)

gemini-code-assist · 2025-09-09T20:24:51Z

testdriver/testplan.py

            self.icu_version = options.icu_version
        except KeyError:
-            logging.warning('NO ICU VERSION SET')
+            logging.warning('testdriver/testplan.py: NO ICU VERSION SET')


Hardcoding the filename prefix in log messages, as done throughout this file, is not ideal for maintainability. If the file is ever renamed, all these log strings would need to be updated manually. A better approach is to obtain the filename dynamically. You can define a constant at the top of the module:

import os FILENAME = os.path.basename(__file__)

And then use it in your log messages, for example:
logging.warning('%s: NO ICU VERSION SET', FILENAME)

gemini-code-assist · 2025-09-09T20:24:51Z

testgen/generators/localeDisplayNames.py

+                        logging.debug('testgen/generator/localeDisplayNames: %s ignored for %s, %s',
+                                     locale_label, test_data[0], language_display)


There seems to be a typo in the hardcoded path: testgen/generator/ should probably be testgen/generators/.

More generally, hardcoding path prefixes in log messages is not ideal for maintainability. It's better to get this information dynamically, for example using __name__ or os.path.basename(__file__).

gemini-code-assist · 2025-09-09T20:24:52Z

testgen/generators/message_fmt2.py

            src_data = self.readFile(test_file_path, filetype="json")
            if src_data is None:
-                logging.error("Problem reading JSON. Omitting file %s", test_file_path)
+                logging.error("testgen/generators/message_fmt2.py: Problem reading JSON. Omitting file %s", test_file_path)


Hardcoding the filename prefix in log messages, as done throughout this file, is not ideal for maintainability. If the file is ever renamed, all these log strings would need to be updated manually. A better approach is to obtain the filename dynamically using os.path.basename(__file__).

gemini-code-assist · 2025-09-09T20:24:52Z

schema/check_schemas.py

            summary_data = json.dumps(summary_json)
        except BaseException as err:
-            logging.error('%s: Cannot create JSON summary: %s', err, summary_json)
+            logging.error('schema/check_schemas: %s: Cannot create JSON summary: %s', err, summary_json)


Hardcoding the filename prefix in log messages, as done throughout this file, is not ideal for maintainability. If the file is ever renamed, all these log strings would need to be updated manually. A better approach is to obtain the filename dynamically. You can define a constant at the top of the module:

import os FILENAME = os.path.basename(__file__)

And then use it in your log messages, for example:
logging.error('%s: %%s: Cannot create JSON summary: %%s', FILENAME, err, summary_json)

gemini-code-assist · 2025-09-09T20:24:52Z

testgen/generators/number_fmt.py


            logging.info(
-                "NumberFormat Test (%s): %s tests created", self.icu_version, count
+                "testgen/generators/number_fmt.py: NumberFormat Test (%s): %s tests created", self.icu_version, count


Hardcoding the filename prefix in log messages, as done throughout this file, is not ideal for maintainability. If the file is ever renamed, all these log strings would need to be updated manually. A better approach is to obtain the filename dynamically using os.path.basename(__file__).

gemini-code-assist · 2025-09-09T20:24:52Z

testgen/generators/plurals.py

            tree = ET.parse(filename)
        except:
-            logging.warning('No plurals file found: %s', filename)
+            logging.warning('testgen/generators/plurals.py: No plurals file found: %s', filename)


Hardcoding the filename prefix in log messages, as done throughout this file, is not ideal for maintainability. If the file is ever renamed, all these log strings would need to be updated manually. A better approach is to obtain the filename dynamically using os.path.basename(__file__).

sffc

This is good progress. I'm eager to see the end-to-end log when CI is passing. It seems you are annotating a lot of the messages in Python, but I think you might also want to annotate the messages generated by the bash scripts as well as any generated by a Python subprocess.

sffc · 2025-09-29T22:13:50Z

genData100.sh

+# To clear warnings
+npm install --package-lock-only


You should run npm only in the package directories that need them

sffc · 2025-09-29T22:15:32Z

generateDataAndRun.sh

+# To clear lock
+npm install --package-lock-only


sffc · 2025-09-29T22:15:53Z

package.json

+{
+  "dependencies": {
+    "@js-temporal/polyfill": "^0.5.1"
+  }
+}


We should have a package.json in each node package directory, but probably not at the top level

sffc

My concerns about the new top-level package.json haven't been addressed

Update logging with info on the source file running

4595f64

gemini-code-assist bot reviewed Sep 9, 2025

View reviewed changes

sven-oly assigned echeran and sffc Sep 9, 2025

This was referenced Sep 9, 2025

Add component or other identifier to logs #417

Open

Error logging: add name of failing piece of the pipeline to errors / warnings / critical #432

Open

gemini-code-assist bot reviewed Sep 9, 2025

View reviewed changes

sffc reviewed Sep 9, 2025

View reviewed changes

Add package.json to fix NPM failure

00bca4b

sffc reviewed Sep 29, 2025

View reviewed changes

sven-oly added 2 commits October 2, 2025 20:57

Change hardcoded file names to __file__. Fix typos

5c8e738

Small change

b2d7fe7

sffc requested changes Oct 9, 2025

View reviewed changes

		logging.error("testgen/generators/message_fmt2.py: JSON %s not validated against schem a %s. Error = %s",
		test_file_path, json_schema_path, error)

		logging.debug('testgen/generator/localeDisplayNames: %s ignored for %s, %s',
		locale_label, test_data[0], language_display)

Uh oh!

Update logging with info on the source file running #531

Are you sure you want to change the base?

Update logging with info on the source file running #531

Uh oh!

Conversation

sven-oly commented Sep 9, 2025 • edited by sffc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

sven-oly commented Sep 9, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

sffc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

sffc Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

sffc Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sven-oly commented Sep 9, 2025 •

edited by sffc

Loading

sffc left a comment •

edited

Loading