-
Notifications
You must be signed in to change notification settings - Fork 230
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
Ignore Paths #186
Ignore Paths #186
Conversation
mentat/include_files.py
Outdated
excluded_files_direct, excluded_files_from_dirs, _ = abs_files_from_list( | ||
exclude_paths, check_for_text=False | ||
) | ||
excluded_files = set( | ||
map(lambda f: f.path, excluded_files_direct | excluded_files_from_dirs) | ||
) | ||
|
||
# Paths excluded by config | ||
glob_excluded_files = set( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, the config's glob_excluded_files only applied to files added by directory (so you can put in in *.json for example in your config to exclude all json files, but if you add a json file manually it will still work). In this PR you kind of merge both --exclude and the glob exclude so that they both only apply to directories, whereas before the --exclude argument would take precedence over manually included files; it might seem weird that we make the --exclude take preference over files you add individually one by one, but because zsh expands globs in the shell, if you make a glob include every file you're including is individual, so now there's no way to exclude files from an include glob (before, the --exclude argument was in place to do exactly this). I do think this is all overly convoluted, so I would be ok with this change (even though I think it's useful to be able to exclude parts of globs), but you need to change the help message for --exclude to make it clear that it only applies to manually added directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha - that was actually a good explanation and I see what you mean. This might be a good excuse to just have a separate argument for auto-context, like 'ignore'.
mentat/include_files.py
Outdated
"""Returns a complete list of text files in a given set of include/exclude Paths.""" | ||
paths = expand_paths(paths) | ||
excluded_files = get_exclude_files(exclude_paths) | ||
|
||
files_direct, files_from_dirs, invalid_paths = abs_files_from_list( | ||
paths, check_for_text=True | ||
) | ||
|
||
# config glob excluded files only apply to files added from directories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would also probably want to change this comment to just all excluded files
Reverted everything to do with |
There was a problem hiding this 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; I will say that I think we might be overcomplicating things a bit having so many different exclude / ignore things, but I do also think we need this so I'm ok with merging it.
There was a problem hiding this 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; I will say that I think we might be overcomplicating things a bit having so many different exclude / ignore things, but I do also think we need this so I'm ok with merging it.
Agreed, it feels like we need a way for users to specify these things, but it is rather complicated! If anyone has a proposal for a more simple approach I'd like to hear it. As long as regular usage is simple and users only need to see these options when they want to do a more complex thing, that seems good to me. As long as it doesn't slow us down trying to support.
This PR fixes the context_benchmark by setting auto_tokens manually (it's default 0 now).
I also realize, as I add benchmarks, that there should be a way to manually exclude files from auto-context, at least in the short term.
EDIT: What we do with exclude_paths is quite complicated (see PCSwingle's comment below), so I added a new argument called
ignore_paths
, which are applied to auto-context. This is analogous to a.mentatignore
file - which might be a good future optimization. Especially if we support non-git projects.