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

fix(chart): fixed path fow Windows #1643

Merged
merged 2 commits into from
Feb 27, 2025
Merged

Conversation

scaliseraoul
Copy link
Contributor

@scaliseraoul scaliseraoul commented Feb 27, 2025


Important

Fixes path handling for chart exports on Windows by using os.path.join for cross-platform compatibility and updates tests accordingly.

  • Behavior:
    • Fixes path handling for DEFAULT_CHART_DIRECTORY in constants.py using os.path.join for cross-platform compatibility.
    • Updates _replace_output_filenames_with_temp_chart in code_cleaning.py to use os.path.join for constructing file paths.
  • Tests:
    • Modifies test_replace_output_filenames_with_temp_chart in test_code_cleaning.py to use regex for path validation.
    • Adds test_create_chart_directory in test_folder.py to verify directory creation with the new path handling.

This description was created by Ellipsis for dffad07. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to dffad07 in 1 minute and 42 seconds

More details
  • Looked at 87 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. pandasai/constants.py:10
  • Draft comment:
    Using os.path.join here correctly handles Windows paths. No issues found.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. pandasai/core/code_generation/code_cleaning.py:222
  • Draft comment:
    Appending a UUID to the chart filename prevents collisions, which is good. Consider updating the docstring for _replace_output_filenames_with_temp_chart to mention that a unique identifier is appended.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. tests/unit_tests/helpers/test_folder.py:15
  • Draft comment:
    The folder test uses os.path.join combined with find_project_root correctly. Ensure that any path normalization required for Windows is consistently handled.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. pandasai/constants.py:10
  • Draft comment:
    Ensure documentation is updated to reflect that DEFAULT_CHART_DIRECTORY is now defined using os.path.join for better cross-platform compatibility.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. pandasai/core/code_generation/code_cleaning.py:218
  • Draft comment:
    Update the function docstring to indicate that the filename now includes a UUID (e.g. 'temp_chart_.png') instead of a static 'temp_chart.png'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. tests/unit_tests/core/code_generation/test_code_cleaning.py:133
  • Draft comment:
    Consider using unittest's assertRegex for regex matching (e.g. self.assertRegex(code, reg)) instead of bare 'assert re.match' for clearer failure messages.
  • Reason this comment was not posted:
    Marked as duplicate.
7. PR Title:1
  • Draft comment:
    Fix the typo in the PR title: 'fow' should be 'for'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_5wiYyzGuwlW9QFZ8


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@gventuri gventuri merged commit fc1fefc into sinaptik-ai:main Feb 27, 2025
12 checks passed
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.

Plot chart failing on windows
2 participants