Add Google-style docstrings and fix bugs in core modules by zaihuaji · Pull Request #91 · NCAR/rda-python-common

zaihuaji · 2026-03-21T13:42:33Z

Summary

pg_dbi.py: Added Google-style docstrings to all 60+ methods; fixed 3 bugs (uninitialized batch dict in pgbatch, wrong cache key in get_specialist, wrong variable name in validate_decs_group)
pg_util.py: Added Google-style docstrings to all 54 methods; fixed 10 bugs including a NameError in get_wday, shadowed built-in str in fmtdatetime, duplicate adddatetime definition, bare integer in for loops in joinarray, float division used as list index in asearch, and attribute-access typo in endtime
pg_file.py: Added Google-style docstrings to all 80+ methods; fixed 6 bugs including dead code after return in remote_copy_local, missing re.match argument in record_delete_directory, wrong files argument in compare_md5sum, op.exist() typo in convert_files, potentially undefined ret in make_one_backup_directory, and wrong os.W_OK permission flag in check_local_executable
pg_sig.py: Added Google-style docstrings to all methods; fixed 5+ bugs including os.sleep/sys.sleep → time.sleep, extra self argument passed to check_process, wrong ckeys index in get_pbs_info, dict mutation during iteration in check_background, and undefined unit variable fallback in get_wait_time

Test plan

Run python3 -m py_compile on all modified files (verified clean)
Run existing test suite: python3 -m pytest
Run linting: python3 -m flake8 . --max-line-length=120
Smoke-test imports in a Python session: from rda_python_common import PgDBI, PgFile, PgUtil, PgSIG

🤖 Generated with Claude Code

- Add class-level and per-method docstrings (Args/Returns) to all methods in PgLOG for better discoverability and IDE support - Fix bug in send_python_email: eml.quit() was called in finally even when smtplib.SMTP() failed, causing a NameError; guard with None check and move success-path logging out of finally - Replace int(x/base) and int(x/10) with // integer division in int2base and base2int - Simplify seconds_to_string_time using divmod instead of manual modulo/division arithmetic - Replace re.search(r'(\n|\r)$', msg) with msg.endswith(('\n', '\r')) - Replace manual open/close file patterns with context managers in log_email, pgdbg, and show_usage - Rename local variable str in get_call_trace to avoid shadowing builtin - Standardise default-argument spacing throughout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- pg_dbi.py: Added docstrings to all 60+ methods; fixed pgbatch uninitialized dict, get_specialist wrong cache key, validate_decs_group wrong variable name - pg_util.py: Added docstrings to all 54 methods; fixed get_wday NameError, fmtdatetime shadowed built-in str, addyearmonth overwritten params, duplicate adddatetime definition, addtime wrong assignment target, joinarray bare int in for loop, asearch float division index, endtime attribute access typo, and two == None identity checks - pg_file.py: Added docstrings to all 80+ methods; fixed remote_copy_local dead code after return, record_delete_directory missing re.match arg, compare_md5sum wrong files arg, convert_files op.exist() typo, make_one_backup_directory undefined ret, check_local_executable wrong os.W_OK permission flag - pg_sig.py: Added docstrings to all methods; fixed os.sleep/sys.sleep -> time.sleep, extra self arg in check_process call, get_pbs_info wrong ckeys index, check_background dict mutation during iteration, get_wait_time undefined unit variable fallback Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR standardizes inline documentation across core RDA Python modules by adding Google-style docstrings, while also incorporating a set of targeted bug fixes in date/time utilities, daemon/process management, logging, and the PostgreSQL DB interface layer.

Changes:

Added Google-style docstrings broadly across PgUtil, PgSIG, PgLOG, and PgDBI methods/classes.
Fixed multiple runtime issues (e.g., sleep calls, indexing bugs, dict mutation during iteration, wrong cache key usage).
Refactored/cleaned up several small logic and robustness issues in utility and DB helper methods.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

File	Description
src/rda_python_common/pg_util.py	Adds docstrings and includes several utility fixes; review found remaining correctness issues in `joinarray`, `addyearmonth`, and `pgnum`.
src/rda_python_common/pg_sig.py	Adds docstrings and fixes multiple daemon/child-process handling bugs (e.g., `time.sleep`, corrected calls).
src/rda_python_common/pg_log.py	Adds docstrings and improves resource handling (e.g., SMTP cleanup); review found a UID-switching assignment bug.
src/rda_python_common/pg_dbi.py	Adds docstrings and fixes multiple DBI issues; review found a broken `check_cdp_wuser()` code path and a robustness issue in `pgbatch()`.

Comments suppressed due to low confidence (1)

src/rda_python_common/pg_dbi.py:1890

check_cdp_wuser() uses pgrec after the initial pgget() returns falsy: it immediately formats pgrec['email'] (and later reads pgrec['cdpid'], pgrec['org_type'], etc.), but pgrec will be None and the initial query only selects wuid anyway. This code path will always raise, so the function needs to fetch the required fields from the appropriate source (or query wuser with the needed columns) before referencing them.

      pgrec = self.pgget("wuser", "wuid", "cdpname = '{}'".format(username), self.PGDBI['EXITLG'])
      if pgrec: return pgrec['wuid']
      idrec = self.pgget("wuser", "wuid", "email = '{}'".format(pgrec['email']), self.PGDBI['EXITLG'])
      wuid = idrec['wuid'] if idrec else 0
      if wuid > 0:

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-21T13:45:52Z

+            for j in range(cnt1):
               if PgUtil.pgcmp(lst1[j], lst2[i]) != 0: break
            if j >= cnt1:
              lst1.append(lst2[i])


In joinarray(..., unique=True), the duplicate-check logic is inverted: the inner loop breaks on the first non-equal element, so items are almost never appended (and the j >= cnt1 check is ineffective because j will end at cnt1-1). This should instead detect whether any element in lst1 equals lst2[i] (break on equality) and append only when no match is found.

Suggested change

for j in range(cnt1):

if PgUtil.pgcmp(lst1[j], lst2[i]) != 0: break

if j >= cnt1:

lst1.append(lst2[i])

is_dup = False

for j in range(cnt1):

if PgUtil.pgcmp(lst1[j], lst2[i]) == 0:

is_dup = True

break

if not is_dup:

lst1.append(lst2[i])

cnt1 += 1

Copilot · 2026-03-21T13:45:52Z

+         if nmn < 0:
+            while nmn < 0:


addyearmonth() fails for cases where the resulting month becomes 0 (e.g., adding -1 month to YYYY01), producing an invalid YYYY00. The normalization should treat nmn <= 0 as underflow (decrement year and add 12) rather than only nmn < 0.

Suggested change

if nmn < 0:

while nmn < 0:

if nmn <= 0:

while nmn <= 0:

Copilot · 2026-03-21T13:45:52Z

+         int: 1 if numeric, 0 otherwise.
+      """
      if not isinstance(val, str): val = str(val)
      ms = re.match(r'^\-{0,1}(\d+|\d+\.\d*|d*\.\d+)([eE]\-{0,1}\d+)*$', val)


The numeric-validation regex uses d*\.\d+ instead of \d*\.\d+, which matches literal 'd' characters rather than digits and will incorrectly reject decimals like .5. This should be corrected so fractional values without a leading zero are recognized.

Suggested change

ms = re.match(r'^\-{0,1}(\d+|\d+\.\d*|d*\.\d+)([eE]\-{0,1}\d+)*$', val)

ms = re.match(r'^\-{0,1}(\d+|\d+\.\d*|\d*\.\d+)([eE]\-{0,1}\d+)*$', val)

Copilot · 2026-03-21T13:45:53Z

@@ -998,8 +1387,16 @@ def set_suid(self, cuid = 0):
            self.set_specialist_environments(self.PGLOG['SETUID'])
            self.PGLOG['CURUID'] == self.PGLOG['SETUID']      # set CURUID to a specific specialist


set_suid() uses == instead of assignment when attempting to update self.PGLOG['CURUID'], so CURUID is never updated after switching UIDs. This is likely unintended and can lead to incorrect identity/authorization logic later in the process.

Suggested change

self.PGLOG['CURUID'] == self.PGLOG['SETUID'] # set CURUID to a specific specialist

self.PGLOG['CURUID'] = self.PGLOG['SETUID'] # set CURUID to a specific specialist

Copilot · 2026-03-21T13:45:53Z

-      pwname = self.get_pgpass_password()
-      os.environ['PGPASSWORD'] = pwname
-      options += " -U {} {}".format(self.PGDBI['LNNAME'], self.PGDBI['DBNAME'])   
+      os.environ['PGPASSWORD'] = self.get_pgpass_password()


pgbatch() sets os.environ['PGPASSWORD'] directly from get_pgpass_password(), which can return None. Assigning a non-string into os.environ raises a TypeError. This should guard against missing credentials (e.g., only set when a password is present, or default to an empty string / raise a controlled error).

Suggested change

os.environ['PGPASSWORD'] = self.get_pgpass_password()

password = self.get_pgpass_password()

if password is not None:

os.environ['PGPASSWORD'] = password

zaihuaji and others added 2 commits March 20, 2026 12:08

Copilot AI review requested due to automatic review settings March 21, 2026 13:42

Copilot started reviewing on behalf of zaihuaji March 21, 2026 13:42 View session

zaihuaji merged commit 9d2b0b0 into main Mar 21, 2026
3 checks passed

Copilot AI reviewed Mar 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Google-style docstrings and fix bugs in core modules#91

Add Google-style docstrings and fix bugs in core modules#91
zaihuaji merged 2 commits intomainfrom
claude/naughty-lehmann

zaihuaji commented Mar 21, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	ms = re.match(r'^\-{0,1}(\d+\|\d+\.\d\|d\.\d+)([eE]\-{0,1}\d+)*$', val)
	ms = re.match(r'^\-{0,1}(\d+\|\d+\.\d\|\d\.\d+)([eE]\-{0,1}\d+)*$', val)

		@@ -998,8 +1387,16 @@ def set_suid(self, cuid = 0):
		self.set_specialist_environments(self.PGLOG['SETUID'])
		self.PGLOG['CURUID'] == self.PGLOG['SETUID'] # set CURUID to a specific specialist

	self.PGLOG['CURUID'] == self.PGLOG['SETUID'] # set CURUID to a specific specialist
	self.PGLOG['CURUID'] = self.PGLOG['SETUID'] # set CURUID to a specific specialist

-      os.environ['PGPASSWORD'] = self.get_pgpass_password()
+      password = self.get_pgpass_password()
+      if password is not None:
+         os.environ['PGPASSWORD'] = password

Conversation

zaihuaji commented Mar 21, 2026

Summary

Test plan

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants