Skip to content

cf_install for all dep managers #71

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cf_install for all dep managers #71

wants to merge 1 commit into from

Conversation

Saga4
Copy link
Contributor

@Saga4 Saga4 commented Mar 25, 2025

User description

image

Next Error : - We should be trying to check if test directory is available or create one to avoid this error.

INFO Logging level set to INFO
────────────────────────────────────────────────────────────────────────────────
Traceback (most recent call last):
File "/home/runner/work/Python/Python/.venv/bin/codeflash", line 10, in
sys.exit(main())
~~~~^^
File "/home/runner/work/Python/Python/.venv/lib/python3.13/site-packages/codeflash/main.py", line 37, in main
args = process_pyproject_config(args)
File "/home/runner/work/Python/Python/.venv/lib/python3.13/site-packages/codeflash/cli_cmds/cli.py", line 129, in process_pyproject_config
assert Path(args.tests_root).is_dir(), f"--tests-root {args.tests_root} must be a valid directory"
~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
AssertionError: --tests-root /home/runner/work/Python/Python/tests must be a valid directory


PR Type

  • Enhancement

Description

  • Standardize formatting and spacing in function definitions.

  • Enhance dependency installation commands for Poetry.

  • Enhance dependency installation commands for uv.

  • Add automatic installation of codeflash package.


Changes walkthrough 📝

Relevant files
Enhancement
cmd_init.py
Update dependency installation commands with codeflash add.

codeflash/cli_cmds/cmd_init.py

  • Fixed spacing in install_github_actions parameter.
  • Updated Poetry commands: added "poetry add codeflash".
  • Updated uv commands: added "uv pip install codeflash".
  • +6/-3     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @Saga4 Saga4 requested a review from aseembits93 March 25, 2025 20:04
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Documentation

    The new function install_github_actions does not include a docstring. Adding proper documentation can improve code clarity and maintainability.

    def install_github_actions(override_formatter_check: bool = False) -> None:
    Command Consistency

    The updated dependency installation commands for POETRY and UV now include additional steps. Ensure that the multiline command strings are consistent and behave as expected across different dependency managers.

          python -m pip install --upgrade pip
          pip install poetry
          poetry install --all-extras
          poetry add codeflash"""
    if dep_manager == DependencyManager.UV:
        return """|
          uv sync --all-extras
          uv pip install codeflash"""

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Chain poetry commands

    Chain the poetry commands with && so that the second runs only if the first
    succeeds.

    codeflash/cli_cmds/cmd_init.py [493-494]

    -poetry install --all-extras
    -poetry add codeflash
    +poetry install --all-extras && poetry add codeflash
    Suggestion importance[1-10]: 5

    __

    Why: Chaining these commands can help ensure that the second command only runs if the first one succeeds, though it’s largely a stylistic choice with moderate impact.

    Low
    Chain uv commands

    Combine the uv commands with && to ensure sequential execution and prevent partial
    installation.

    codeflash/cli_cmds/cmd_init.py [496-497]

    -uv sync --all-extras
    -uv pip install codeflash
    +uv sync --all-extras && uv pip install codeflash
    Suggestion importance[1-10]: 5

    __

    Why: Combining the uv commands with && will enforce sequential execution to avoid partial installation, representing a minor improvement in command reliability.

    Low

    return "uv sync --all-extras"
    return """|
    uv sync --all-extras
    uv pip install codeflash"""
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    it is expected that codeflash is already added in the dev time dependency. We should not re-install codeflash...

    Copy link
    Contributor

    @aseembits93 aseembits93 Mar 25, 2025

    Choose a reason for hiding this comment

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

    try doing 'uv run' with absolute path? the path you get after running which codeflash

    @@ -490,9 +490,12 @@ def get_dependency_installation_commands(dep_manager: DependencyManager) -> tupl
    return """|
    python -m pip install --upgrade pip
    pip install poetry
    poetry install --all-extras"""
    poetry install --all-extras
    poetry add codeflash"""
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    this modifies the dependencies spec, and adds codeflash in the required set of dependencies. This is wrong to do in the CI

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    why is it wrong to do so?

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

    Successfully merging this pull request may close these issues.

    3 participants