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

Improve coverage aedt_file_management file #5756

Open
Samuelopez-ansys opened this issue Feb 10, 2025 · 4 comments · May be fixed by #5879
Open

Improve coverage aedt_file_management file #5756

Samuelopez-ansys opened this issue Feb 10, 2025 · 4 comments · May be fixed by #5879
Assignees
Labels
enhancement New features or code improvements

Comments

@Samuelopez-ansys
Copy link
Member

Description of the current limitations

The mentioned file is not tested

Code sample expected

No response

Useful links and references

No response

@Samuelopez-ansys Samuelopez-ansys added the enhancement New features or code improvements label Feb 10, 2025
@Samuelopez-ansys Samuelopez-ansys self-assigned this Feb 10, 2025
@SMoraisAnsys
Copy link
Collaborator

@Marwane-20 here's one of the tasks I'd like you to tackle. Also, it would be nice if you could improve the code and propose refactoring (not changed the method name and signature, only the implementation if it makes sense).

@Marwane-20
Copy link

@SMoraisAnsys understood, i will get right on that

@Marwane-20
Copy link

Marwane-20 commented Feb 16, 2025

Hello @SMoraisAnsys, here are my suggestions I plan to implement:

1. Consistency in Return Values

  • Both change_objects_visibility and change_model_orientation have docstrings indicating a boolean return value, but they don’t explicitly return anything.
    I propose adding explicit True (or False in error cases) returns.

2. Function Improvements

read_info_fromcsv(projdir, name)

  • Variable Naming:
    Rename the variable listmcad to more descriptive like rows .

  • File Mode Consideration:
    Since CSV files are text, we might consider opening the file in text mode (using the proper encoding) unless there’s a specific reason for binary mode.

clean_proj_folder(dir, name)

  • Unused Parameter:
    name parameter is not used. Can't change signature.

create_output_folder(ProjectDir)

  • Directory Creation:
    Simplify the directory creation using os.makedirs(..., exist_ok=True) instead of multiple if not os.path.exists(...) checks.

  • Code Duplication:
    Extract the repeated use of os.path.basename(npath) into a variable to reduce redundancy and improve readability.

change_objects_visibility(origfile, solid_list) AND change_model_orientation(origfile, bottom_dir)

  • File Handling & Encoding:
    Currently, the file is opened in binary mode while performing string operations. maybe decode the bytes into a string with a defined encoding (e.g., UTF-8) or open in text mode.

  • Temporary File Handling:
    Consider using Python’s tempfile module for managing temporary files more securely.

  • Regex Substitution Validation:
    Add logic to confirm that the regex substitution has occurred as expected before proceeding with file replacement.

Other/Questions :

There is @pyaedt_function_handler decorator for overall exception handling, there are some file operations where a more granular try/except might be beneficial. For example in change_objects_visibility or change_model_orientation:

            if not os.path.isfile(origfile + ".lock"):  # check if the project is closed
                    with open(origfile, "rb") as f, open(newfile, "wb") as n:
                        # Reading file content
                        content = f.read()
            
                        # Searching file content for pattern
                        pattern = re.compile(
                            r"(\$begin 'EditorWindow'\n.+)(Drawings\[.+\])(.+\n\s*\$end 'EditorWindow')", re.UNICODE
                        )
                        # Replacing string
                        # fmt: off
                        view_str = u"Drawings[" + str(len(solid_list)) + u": " + str(solid_list).strip("[")
                        s = pattern.sub(r"\1" + view_str + r"\3", content)
                        # fmt: on
                        # Writing file content
                        n.write(str(s))
            
                    # Renaming files and deleting temp file
                    os.remove(origfile)
                    os.rename(newfile, origfile)
        
            else:  # project is locked
                print("change_objects_visibility: Project %s is still locked." % origfile)

In this function, if there is a file permission issue or a failure during the file write, removal, or rename—the temporary file (newfile) may not be handled correctly. This could leave the file system in an inconsistent state (e.g., the original file might be removed while the new file isn’t properly renamed).

Should we add localized try/except blocks around the critical file operations (like reading, writing, removing, and renaming) to better handle errors such as permission issues or I/O failures?

@SMoraisAnsys
Copy link
Collaborator

I like the idea, please propose your changes and we'll evaluate that afterward :)

@Samuelopez-ansys Samuelopez-ansys linked a pull request Mar 7, 2025 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants