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

Remove git root from parsers #347

Merged
merged 63 commits into from
Dec 8, 2023
Merged

Remove git root from parsers #347

merged 63 commits into from
Dec 8, 2023

Conversation

waydegg
Copy link
Contributor

@waydegg waydegg commented Dec 4, 2023

This PR is part of a series of PRs to remove git_root from Mentat, and allow Sessions to be run anywhere on the filesystem.

@@ -99,7 +99,9 @@ async def test_run_from_subdirectory(
# Hello
@@end""")])

session = Session(cwd=Path.cwd(), paths=[Path("calculator.py"), Path("../scripts")])
session = Session(
cwd=temp_testbed, paths=["multifile_calculator/calculator.py", "scripts"]
Copy link
Contributor Author

@waydegg waydegg Dec 4, 2023

Choose a reason for hiding this comment

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

paths should be relative to cwd, so this testcase along with a few others were updated

@@ -167,12 +167,12 @@ def _special_block(
if deserialized_json.action == _BlockParserAction.Delete:
replacements.append(Replacement(starting_line, ending_line, []))
file_edit = FileEdit(
git_root / deserialized_json.file,
Copy link
Member

Choose a reason for hiding this comment

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

So the model is going to be outputting paths relative to the cwd instead of git_root now, right? That sounds good to me, just wanted to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

@@ -57,8 +57,7 @@ def __init__(

# Since we can't set the session_context until after all of the singletons are created,
# any singletons used in the constructor of another singleton must be passed in
# TODO: An error is thrown in this function; once git root is removed, the error will be removed
git_root = get_shared_git_root_for_paths([Path(path) for path in paths])
Copy link
Member

Choose a reason for hiding this comment

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

In config.py on line 150 we use this function to get the git root so that we can check for the project config; maybe you were already planning to do this in another PR, but the get_shared_git_root function will currently crash. I think we could change it to the get_git_root_for_path function and we could first check for a project config in the git root and then in the cwd if there is not git root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could change it to the get_git_root_for_path function and we could first check for a project config in the git root and then in the cwd if there is not git root.

Agree on this being the behavior for determining the config to use. I'll add this in another PR!

Copy link
Member

@PCSwingle PCSwingle left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Base automatically changed from remove-git-root-6 to main December 8, 2023 02:02
@waydegg waydegg merged commit 47b383e into main Dec 8, 2023
16 checks passed
@waydegg waydegg deleted the remove-git-root-7 branch December 8, 2023 02:47
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