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

Path replace bug #2549

Closed
wants to merge 4 commits into from
Closed

Conversation

rudrakshnalbalwar
Copy link

Type : Bug Fix

Description: Addressing Inconsistent File Path Handling in tardis
Description:

This pull request fixes a bug in tardis related to how file paths are handled in tardis/io/atom_data/util.py and potentially other parts of the codebase.

Problem:

The variable fname in resolve_atom_data_fname can be either a string representing a filename or a path object. When fname is a path object, attempting to use the replace method on line 38 leads to an error because path objects have their own replace method. This inconsistency in data types causes unexpected behavior.

Solution:

This pull request implements the following fix:

Explicit Type Conversion (Recommended): The code is modified to ensure fname is always a string before applying the replace method. This is achieved by checking if fname is an instance of os.PathLike (a common base class for path-like objects) and converting it to a string using str(fname).
Benefits:

Fixes the bug causing errors when fname is a path object.
Improves code clarity and maintainability by ensuring consistent data types.
Testing:

Unit tests (if available) should be updated to verify the fix.
Manual testing can be performed to ensure the functionality of resolve_atom_data_fname and related functions is not affected.
Additional Notes:

Similar issues in other parts of the codebase (e.g., tardis/simulation/base.py) might need to be addressed.
Consider using type hints (PEP 484) to explicitly define expected data types for function arguments and return values.

Please review this pull request and let me know if you have any questions.

@rudrakshnalbalwar rudrakshnalbalwar changed the title Bugs Path replace bug Mar 15, 2024
@rudrakshnalbalwar rudrakshnalbalwar marked this pull request as draft March 16, 2024 15:57
@rudrakshnalbalwar rudrakshnalbalwar marked this pull request as ready for review March 16, 2024 16:07
@rudrakshnalbalwar rudrakshnalbalwar marked this pull request as draft March 16, 2024 16:19
@rudrakshnalbalwar rudrakshnalbalwar marked this pull request as ready for review March 16, 2024 18:38
@andrewfullard
Copy link
Contributor

Thanks for the submission. We went with #2535 instead, but you can still refer to this PR in your GSoC application as a reference for your code.

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.

2 participants