Skip to content

fix: code quality and type safety improvements#815

Open
caverac wants to merge 1 commit intojobovy:mainfrom
caverac:fix/features-improvements
Open

fix: code quality and type safety improvements#815
caverac wants to merge 1 commit intojobovy:mainfrom
caverac:fix/features-improvements

Conversation

@caverac
Copy link
Contributor

@caverac caverac commented Jan 6, 2026

This PR

  • implements code quality improvements
  • adds type safety enhancements
  • refactors to consolidate unit conversion handling across the codebase.

1. Python Improvements

Bare Except Clauses Replaced with Specific Exceptions

Standard: PEP 8 - Programming Recommendations

"When catching exceptions, mention specific exceptions whenever possible instead of using a bare except: clause."

Bare except: clauses catch all exceptions which can mask bugs, prevent proper exception handling, hide errors and make debugging difficult.

# before
except:
    pass

# now
except ImportError:
    pass

Comparison Operator Style Fixed

Standard: PEP 8 - Programming Recommendations

"Comparisons to singletons like None should always be done with is or is not, never the equality operators."

The negation pattern not x is y is less readable than x is not y. PEP 8 explicitly recommends using the latter form for clarity.

# before 
if not ro is False:
    if not ro is None:

# now
if ro is not False:
    if ro is not None:

Import Aliases Standardized

Standard: PEP 8 - Imports and community conventions

For maximum clarity in a scientific library, using the full import numpy with numpy. prefix was chosen to be explicit.

# before
import numpy as nu
nu.array([1, 2, 3])

# now
import numpy
numpy.array([1, 2, 3])

Unnecessary return None Removed

Standard: PEP 8 - Programming Recommendations

"Be consistent in return statements. Either all return statements in a function should return an expression, or none of them should. If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None, and an explicit return statement should be present at the end of the function (if reachable)."

For functions that don't return a value (procedures), Python implicitly returns None.

# before
def turn_physical_off(self):
    self._roSet = False
    self._voSet = False
    return None

# now
def turn_physical_off(self):
    self._roSet = False
    self._voSet = False

Pythonic Iteration Patterns

Standard: PEP 20 - The Zen of Python

"There should be one-- and preferably only one --obvious way to do it."

The range(len(x)) pattern is a common anti-pattern in Python. Using enumerate() for index-value pairs and zip() for parallel iteration is more Pythonic, readable, and less error-prone.

# before
for ii in range(len(q)):
    result[ii] = process(q[ii], p[ii])

# now
for ii, (qi, pi) in enumerate(zip(q, p)):
    result[ii] = process(qi, pi)

2. Typing Improvements

PEP 561 Type Checker Support

Standard: PEP 561 - Distributing and Packaging Type Information

Added py.typed marker file to indicate that the package supports type checking. This allows type checkers like mypy, pyright, and IDEs to recognize that galpy provides type information.

"Package maintainers who wish to support type checking of their code MUST add a marker file named py.typed to their package."

Type Annotations

Standards:

Added type hints to core module functions and methods. Type hints provide:

  1. Documentation: Types serve as machine-checkable documentation
  2. IDE Support: Better autocomplete, refactoring, and error detection
  3. Static Analysis: Catch type errors before runtime with tools like mypy
  4. Maintainability: Makes code contracts explicit for future developers
# before
def __init__(self, ro=None, vo=None):
    ...

# now
def __init__(
    self, ro: Optional[float] = None, vo: Optional[float] = None
) -> None:
    ...

3. Refactor

ConversionContext Class for DRY Parameter Handling

Principles:

Created a ConversionContext class to address the repetitive (vo, ro) parameter passing across 15+ conversion functions. This follows the DRY principle by allowing users to set conversion parameters once and reuse them.

Problem: Every conversion function required passing the same vo and ro parameters:

# Before - repetitive and error-prone
dens = dens_in_msolpc3(vo, ro)
mass = mass_in_msol(vo, ro)
time = time_in_Gyr(vo, ro)
freq = freq_in_Gyr(vo, ro)
force = force_in_kmsMyr(vo, ro)

Solution: Context object stores parameters once:

# After - clean and maintainable
ctx = ConversionContext(ro=8.0, vo=220.0)
dens = ctx.dens_in_msolpc3()
mass = ctx.mass_in_msol()
time = ctx.time_in_Gyr()
freq = ctx.freq_in_Gyr()
force = ctx.force_in_kmsMyr()

# Or create directly from a galpy object
ctx = ConversionContext.from_object(MWPotential2014)

This refactor:

  • Reduces code duplication at call sites
  • Prevents parameter mismatch errors (using different ro/vo accidentally)
  • Provides a cleaner, more object-oriented API
  • Maintains full backward compatibility with existing function-based API

@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 73.80952% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.84%. Comparing base (06943f2) to head (5d84fd4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
galpy/util/conversion.py 57.69% 22 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #815      +/-   ##
==========================================
- Coverage   99.91%   99.84%   -0.08%     
==========================================
  Files         213      213              
  Lines       30740    30781      +41     
  Branches      627      627              
==========================================
+ Hits        30713    30732      +19     
- Misses         27       49      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant